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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
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);
----------------
steakhal wrote:
> 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.
Okay, I've added the following comment (though this feels a bit too mouthful for me):
```
/// ... We use this function in the family of assumeSymNE/EQ/LT/../GE
/// functions where we can work only with symbols. Use the other function
/// (simplifyToSVal) if you are interested in a simplification that may yield
/// a concrete constant value.
```


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2110
+
+    SymbolRef SimplifiedMemberSym = nullptr;
+    SVal SimplifiedMemberVal = simplifyToSVal(State, MemberSym);
----------------
steakhal wrote:
> 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.
Good point! I've made the suggested change.


================
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();
----------------
steakhal wrote:
> 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.
Ok, I've added a comment for that.


================
Comment at: clang/test/Analysis/solver-sym-simplification-concreteint.c:17
+    return;
+  clang_analyzer_warnIfReached(); // expected-no-diagnostics
+
----------------
steakhal wrote:
> 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.
Okay, makes sense. I've added a new test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110913