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

[Denys Petrov via Phabricator via cfe-commits]

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

@baloghadamsoftware

> However, please still extend the current tests

I've looked for some appropriate tests to extend, but haven't found. What would you advise to use instead of mine?
@steakhal
Thanks for the comments.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();
----------------
steakhal wrote:
> Should we take it as `const ref` to prevent copying?
getMinValue returns APSInt by value, so it wouldn't make sense.


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


================
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}}
+}
----------------
steakhal wrote:
> What does this test case demonstrate? Could you elaborate on that?
> Why do you evaluate the `x - y != 0` here twice?
>Why do you evaluate the x - y != 0 here twice?
This is the root line which assertion occured on.
I'll add some comments.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23
+template <typename T> struct TestCase {
+  RangeSet original;
+  RangeSet expected;
----------------
steakhal wrote:
> 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).
Here is a discussion [[ https://llvm.org/docs/Proposals/VariableNames.html | VariableNames]] I was guide. Seems like we can use camelBack for new code. Am I right? Also RangeSetTest.cpp already uses mixed naming as you can see.


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


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


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66
+  template <typename T> void checkNegate() {
+    using type = T;
+
----------------
steakhal wrote:
> To be honest, I'm not sure if `type` is more descriptive than `T`.
It is useful for debug purposes, for instance you can change it to `using type = int;` to verify something. It also untie from template argument list and make the code more flexible.


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


================
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());
----------------
steakhal wrote:
> Should we keep this?
I'm not sure, but I'd better keep it, because it is useful for debugging.


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

https://reviews.llvm.org/D77802