[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 4 inline comments as done.
ASDenysPetrov added a comment.

Thank you for the feedback. I'll update the patch soon.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:261-262
+    // remove adjacent ranges
+    newRanges = F.remove(newRanges, *iter1);
+    newRanges = F.remove(newRanges, *iter2);
+    // add united range
----------------
NoQ wrote:
> I don't remember iterator invalidation rules for these containers and it gives me anxiety. Given that these containers are immutable, i wouldn't be surprised if iterators are indeed never invalidated (just keep pointing to the old container) but it definitely deserves a comment.
Thanks for the notice. I'm not sure about iterator invalidation, so I'll just replace `*iter2` with `*newRanges.begin()`.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+           const std::initializer_list<T> &originalList,
+           const std::initializer_list<T> &expectedList)
+      : original(createRangeSetFromList(BVF, F, originalList)),
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > AFAIK since `std::initializer_list` is just two pointers we should take it by value.
> > I'm not sure about //should//, since initializer_list is a container and it'd be consistent if we handle it like any other compound object despite its implementation (IMO). But I can change it if you wish.
> > `initializer_list` is a container
> 
> No, it's a //view// :)
> No, it's a view :)
Yes, you are right. OK, I'll change it to value argument.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Generally, `new expressions` are a code smell. We should use something like an `std::make_unique` to prevent memory leaks on exceptions.
> > > Though, I'm not sure if there is a similar function for `llvm::IntrusiveRefCntPtr<T>`s.
> > I'll make it more safe.
> I'd rather create ASTContext through tooling, like in other unittests.
I'll look into other tests and try to change to ASTContext.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+      // c.original.print(llvm::dbgs());
+      // llvm::dbgs() << " => ";
+      // c.expected.print(llvm::dbgs());
+      // llvm::dbgs() << " => ";
+      // negatedFromOriginal.print(llvm::dbgs());
+      // llvm::dbgs() << " => ";
+      // negatedBackward.print(llvm::dbgs());
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Should we keep this?
> > I'm not sure, but I'd better keep it, because it is useful for debugging.
> No-no, we don't do that here >.>
> 
> If you constructed an awesome debugging facility and you want to save it for future generations, i'd recommend turning it into an actual facility, not just comments. Like, maybe, an unused function that can be invoked for debugging, or something like that.
OK, I'll move it to function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77802/new/

https://reviews.llvm.org/D77802