[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. Is the syntax of specifying expected notes and warnings documented somewhere? I could not find the note-specific syntax. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. The issues highlighted here have been partially (one could argue mostly) solved by D105821 . That patch hasn't been merged because it needs more tests and also needs to be split. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. > So here you are implying that for reinterpret casts, the > `CastE->path_begin()-end()` range is empty? And that is why you need to > manually come up with a //possible// CXXBaseSpecifier list? @steakhal, Yes. For illustration, the following code: struct Base {

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326731. RedDocMD added a comment. Replaced BFS with existing CXXRecordDeclMethod Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAna

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, you are absolutely right! It works. Thank you for pointing it out, not sure how I missed this earlier this evening. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326733. RedDocMD added a comment. Removed unnecessary includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAnalyzer/Core/PathSen

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD marked an inline comment as done. RedDocMD added a comment. In D97183#2589445 , @NoQ wrote: > Nice! > > I suspect you're adding too many notes. The note needs to not be there if the > *raw* pointer is not tracked. Eg., I suspect that your patch w

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:43-50 +struct A {}; +struct B : public A {}; +struct C { + int field; +}; +struct D : public C {}; +struct E : public B, public D {};

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, I guess I would need to figure out a way to find out if the raw pointer obtained from `get()` is being used somewhere or is being constrained. I am trying to first figure out whether the raw pointer is being constrained to null, causing a null-deref to detected.

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 327396. RedDocMD added a comment. Should not emit note if raw pointer cannot be tracked Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyz

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:20 int DoubleDerived::*ddf = &Base::field; int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf))); Base ba

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. In D97183#2596865 , @steakhal wrote: > If the inner pointer participates in a branch condition guarding the > dereference, that memory region is gotta be important, right? So, we should > mark it so. > A BugreportVisitor could e

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31 + Some some; + some.*sf = 14; + clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}} steakhal

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62 +void testMultiple() { + int F::*f = &F::field; + int A::*a = reinterpret_cast(f); + int C::*c = reinterpret_cast(f); + A aobj; + C cobj; + aobj.*a = 13; ---

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. Talking on the mailing list, I got a link on reinterpret_casting for pointer-to-member: https://timsong-cpp.github.io/cppwp/n4861/expr.reinterpret.cast#10 The gist is that this sort of cast is only valid if we cast back to the original type (that too with a caveat). N

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. Sorry, was a bit caught up with assignments. I will try to come up with a better implementation with the advice given by @NoQ and @steakhal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.or

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. In D96976#2605815 , @steakhal wrote: > What is that caveat? The second point in the link I gave above. > Do you have anything specific in mind about implementing the 3rd option? > Why do you think it's //significantly complex//?

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 328931. RedDocMD added a comment. Replaced smart-ptr dereference with direct access Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/Static

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. In D97183#2598806 , @NoQ wrote: > We could, for instance, teach it to mark //exploded nodes// as interesting > when it changes tracking mode. @NoQ, what is tracking mode for an `ExplodedNode`? Repository: rG LLVM Github Mono

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. I am trying to use scan-build on a file to see what sort of errors are reported: `./llvm-project/release/bin/scan-build -o . -enable-checker alpha.cplusplus.SmartPtr -analyzer-config alpha.cplusplus.SmartPtrModelling:ModelSmartPtrDereference=true clang++ -c uniq_deref

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. In D97183#2615096 , @NoQ wrote: >> why does this not work? > > How does this not work? What does it say? Sorry, my bad! I had made a typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. Your view, @steakhal? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, looking through the source code of `trackExpressionValue` I can see that it adds many visitors to the `BugReport` passed to it. That I believe is the recursive attachment of visitors you described above. So, as far as I understood, I have to make changes in this f

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 329989. RedDocMD added a comment. Calling trackExpressionValue to mark InnerPointerVal as interesting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/li

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, I am using `trackExpressionValue` to add intersetingness to the `InnerPointerVal`. That solves the original problem. However, it is causing the **MoveChecker** to add extra warnings to use-after-move cases. Essentially, when a `unique_ptr` is moved and subsequent

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, your views? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. In D96976#2622342 , @steakhal wrote: > I'm somewhat busy. If it's not urgent, I would postpone this. > Ping me in a few weeks. Sure ok. Is it okay if I tell someone else to take a look at this in the meanwhile? Repository: r

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 330422. RedDocMD added a comment. Removed an embarassingly dumb mistake Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/Smar

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, sorry for the absurdly dumb mistake. Not entirely sure what I was thinking. Can you please have a look at it now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, could you please have a look at this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. I think something is wrong about the way we are investigating this or I don't understand the `MallocChecker`. The following doesn't yield a bug report! => https://godbolt.org/z/Y57G7zE5j Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. Okay, this is alarming. The following code yields a leak warning: #include #include struct A { int field; A(int f = 0) : field{f} {} void foo() { std::cout << "Field: " << field << "\n"; } }; void foo() { int *raw = new int(10); std:

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. F16456099: 2021-04-24_733x821.png As you can see here, the bug note is attached in the second case (please refer to my previous comment), but somehow, it doesn't finally show up as a bug. Weird. Repository: rG LLVM Github Monorep

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. The call to `PathSensitiveBugReport::markInvalid()` is triggered by `LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor()`. So I guess we need to see now why the visitor thinks this is a false positive. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. Judging by this line in the `LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor()` method, it seems that the bug repor

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. In D98726#2721228 , @NoQ wrote: >> when the visitor encounters an `ExplodedNode` > > Weird. `finalizeVisitor()` accepts not any node but the error node. Your > screenshot suggests that the error node is not in the standard librar

[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

RedDocMD added a comment. In D98726#2721228 , @NoQ wrote: >> when the visitor encounters an `ExplodedNode` > > Weird. `finalizeVisitor()` accepts not any node but the error node. Your > screenshot suggests that the error node is not in the standard librar

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D98726: [analyzer] Remove unnecessary TODO

RedDocMD created this revision. RedDocMD added reviewers: NoQ, vsavchenko, steakhal. Herald added subscribers: ASDenysPetrov, martong, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. RedDocMD requested review of this revision. Herald adde

[PATCH] D98726: [analyzer] Remove unnecessary TODO

RedDocMD updated this revision to Diff 331164. RedDocMD added a comment. Removed unnecessary include Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98726/new/ https://reviews.llvm.org/D98726 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeli

[PATCH] D98726: [analyzer] Remove unnecessary TODO

RedDocMD added a comment. @NoQ, regarding https://godbolt.org/z/1EczEW, I don't think there should be a warning since the original pointer may be needed in the caller. But thanks for pointing out the possible leaks due to release! :) In the line that you have pointed out, I have thought about the

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, could you please have a look at this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, being picky with code from beginners is a good way to train them to write code I think, and so I must thank you for you scrutiny! As for the test you pointed out, it is a wrong test. It is wrong now that I have a better understanding of the problem. There are

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 331461. RedDocMD added a comment. Added a negative test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. > Does the following test work? @NoQ, it seems to be working. > So you're saying that simply by always tracking the (final) raw pointer value > and checking whether the raw value is interesting upon .get() you dodge the > communication problem entirely I would not sa

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 331469. RedDocMD added a comment. Added some more positive tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrChe

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332147. RedDocMD added a comment. Fixed up the git history Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.c

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:317 void getShouldNotAlwaysLeaveANote() { std::unique_ptr P; // expected-note {{Default constructed smart pointer 'P' is null}} ---

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332148. RedDocMD marked an inline comment as done. RedDocMD added a comment. Re-formatted file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/Stati

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332193. RedDocMD added a comment. Added some more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, why does the following trigger a null-dereference warning? (https://godbolt.org/z/Kxox8qd16) void g(std::unique_ptr a) { A *aptr = a.get(); if (!aptr) {} a->foo(); } When `a->foo()` is called, the constraint `!aptr` is no longer valid and so `Inn

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332538. RedDocMD added a comment. Changed approach to use visitor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332540. RedDocMD added a comment. Repaired git history Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h clang/li

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 332542. RedDocMD added a comment. Removed extra includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h clang/

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, I have taken a different approach this time. I have used a visitor and am storing some more data in the GDM. Together, they distinguish between the following three cases: 1. If the raw pointer obtained from get() is constrained to null in a path which leads to a

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD marked 2 inline comments as done. RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:87 + const ProgramStateRef BugState; + llvm::SmallPtrSet DoneExprs; + steakhal wrote: > I'm not sure if we should expect

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD marked 3 inline comments as done. RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:106-109 +if (const auto *DS = llvm::dyn_cast(S)) { + for (const Decl *D : DS->getDeclGroup()) { +if (const VarDecl *VD = l

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD marked an inline comment as done. RedDocMD added a comment. @NoQ, what do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-commits mail

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. Right, sorry for the late reply, @NoQ. I will get to it once I get these assignments off my head. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 __

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. For the following function: void foo(std::unique_ptr P) { A* praw = P.get(); A* other = praw; if (other) {} P->foo(); } Where do we expect a note? Where `praw` is initialized, where `other` is initialized or both? Repository: rG LLVM Github Mono

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. > It's important to realize that with pure static analysis it is absolutely > impossible to reliably report a bug more severe than dead code. Any form of > static analysis only ever finds code that doesn't make sense. It cannot make > assumptions about how often the co

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD updated this revision to Diff 338933. RedDocMD added a comment. Changed to a different visitor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ, I have changed to the visitor that you suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-commits mailing list cfe

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

RedDocMD added a comment. @NoQ? (I actually should remove some extra includes and extra member fields) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 ___ cfe-com

[PATCH] D108912: [release][analyzer] Add 13.0.0 release notes

RedDocMD added a comment. I suppose that the `SmartPtrModelling` patches from GSoC this year shouldn't be added since they only involve an alpha checker. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108912/new/ https://reviews.llvm.org/D108912

[PATCH] D108912: [release][analyzer] Add 13.0.0 release notes

RedDocMD added a comment. In D108912#2982201 , @Szelethus wrote: > In D108912#2982169 , @RedDocMD > wrote: > >> I suppose that the `SmartPtrModelling` patches from GSoC this year shouldn't >> be added since they

<    1   2   3