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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm impressed.
Though, I had some nits, please don't take it to heart :)

And consider joining the to the pre-merge beta testing <https://reviews.llvm.org/project/view/78> project to benefit from buildbots and much more - for free.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();
----------------
Should we take it as `const ref` to prevent copying?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:235
+    // handle a special case for MIN value
+    if (isFromMinValue) {
+      // if [from, to] are [MIN, MAX], then just return the same [MIN, MAX]
----------------
AFAIK in a `RangeSet` the ranges are sorted by the `From` point in ascending order.
We should not check it for each range, only for the first.

Also, note that small and flat loops are more readable.
Consider using //pure// lambdas to reduce the body of the loop and of course to bind algorithms to names.


================
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;
----------------
Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to something similar?


================
Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:114-118
+void negated_unsigned_range(unsigned x, unsigned y) {
+  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+}
----------------
What does this test case demonstrate? Could you elaborate on that?
Why do you evaluate the `x - y != 0` here twice?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23
+template <typename T> struct TestCase {
+  RangeSet original;
+  RangeSet expected;
----------------
According to the [LLVM coding style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) we should use UpperCamelCase for variable names for new code.

Note that the Analyzer Core was written in a different style, we should follow that there instead (at least IMO).


================
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)),
----------------
AFAIK since `std::initializer_list` is just two pointers we should take it by value.


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


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66
+  template <typename T> void checkNegate() {
+    using type = T;
+
----------------
To be honest, I'm not sure if `type` is more descriptive than `T`.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:68
+
+    // use next values of the range {MIN, A, B, MID, C, D, MAX}
+
----------------
AFAIK full sentences are required for comments.
https://llvm.org/docs/CodingStandards.html#commenting


================
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());
----------------
Should we keep this?


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

https://reviews.llvm.org/D77802