[PATCH] D110913: [analyzer][solver] Handle simplification to ConcreteInt

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

steakhal added a comment.

Looks good to me. I very much like this.
Check my nits inline. Given those are fixed I'm gonna accept this.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:392-397
+/// Try to simplify a given symbolic expression based on the constraints in
+/// State. This is needed because the Environment bindings are not getting
+/// updated when a new constraint is added to the State. If the symbol is
+/// simplified to a non-symbol (e.g. to a constant) then the original symbol
+/// is returned.
 SymbolRef simplify(ProgramStateRef State, SymbolRef Sym);
----------------
Okay, I like it!
However, it's still not entirely clear to me *when* to use which.
Could you clarify that aspect too?
Sorry for being picky but I'm not an expert on this part of the code and if it's not clear to me, it probably won't be clear to newcomers either.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2110
+
+    SymbolRef SimplifiedMemberSym = nullptr;
+    SVal SimplifiedMemberVal = simplifyToSVal(State, MemberSym);
----------------
By initializing it here, it will be much cleaner:
1) It's not mutated, thus it can be `const`
2) We can spare the `else` branch.

Also consider marking the rest of the variables `const`, so that the lack of constness would actually suggest mutability.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2112
+    SVal SimplifiedMemberVal = simplifyToSVal(State, MemberSym);
+    if (const auto CI = SimplifiedMemberVal.getAs<nonloc::ConcreteInt>()) {
+      const llvm::APSInt &SV = CI->getValue();
----------------
I think a comment would be appreciated describing that we only look for infeasibility here, nothing else.
Thus, the //fallthrough// in the feasible case is intentional.


================
Comment at: clang/test/Analysis/solver-sym-simplification-concreteint.c:17
+    return;
+  clang_analyzer_warnIfReached(); // expected-no-diagnostics
+
----------------
Consider following the convention and using the `// no-warning` comment instead.
I'm also requesting an additional test case, exercising the fallthrough behavior I stated in an earlier comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110913