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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The math looks correct, thank you for fixing the crash and adding support for more cases!



================
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
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573
       if (const RangeSet *negV = State->get<ConstraintRange>(negSym)) {
-        // Unsigned range set cannot be negated, unless it is [0, 0].
-        if ((negV->getConcreteValue() &&
-             (*negV->getConcreteValue() == 0)) ||
+        if (T->isUnsignedIntegerOrEnumerationType() ||
             T->isSignedIntegerOrEnumerationType())
           return negV;
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to something similar?
> I've looked for similar single function, but there is no appropriate one. I decided not to add a new one to make patch more focused.
`isIntegralOrEnumerationType()`, i guess.


================
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)),
----------------
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// :)


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;
----------------
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.


================
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());
----------------
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.


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

https://reviews.llvm.org/D77802