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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
