martong marked 4 inline comments as done.
martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:44-49
+ // Handle adjustment with non-comparison ops.
+ const llvm::APSInt &Zero = getBasicVals().getValue(0, SIE->getType());
+ if (Assumption) {
+ return assumeSymRel(State, SIE, BO_NE, Zero);
+ }
+ return assumeSymRel(State, SIE, BO_EQ, Zero);
----------------
ASDenysPetrov wrote:
> Actually what you are trying to do here is already inside
> `assumeSymUnsupported` and it will work for all `SymIntExpr`.
> Please, proveide a test case which works with `assumeSymRel` and doesn't with
> `asume` from the previous change. That is what we are trying to fix here.
Thanks for suggesting the use of the ternary op, the code is way more elegant
that way!
> Actually what you are trying to do here is already inside
> `assumeSymUnsupported` and it will work for all `SymIntExpr`.
I see your point and I acknowledge that this new code resembles to the one in
`assumeSymUnsupported`. However, there is a subtle but very important
difference. In `assumeSymUnsupported` we use assumeSym**NE/EQ**, but here I use
assumeSym**Rel**. And assumeSym**Rel** does compute the adjustment, but
assumeSym**NE/EQ** do not. Also, it makes sense to compute the adjustment only
in case of `SymIntExpr`s, thus this could not be done in `assumeSymUnsupported`.
> Please, proveide a test case which works with `assumeSymRel` and doesn't with
> `asume` from the previous change. That is what we are trying to fix here.
You are right, it desires a test case, so I just added the one I had pasted
here previously (with `(x + y + 1) % 3`). If you comment-out the modifications
here in `assumSym` then that test case will fail.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:52
+
+ if (const SymSymExpr *SSE = dyn_cast<SymSymExpr>(Sym)) {
BinaryOperator::Opcode Op = SSE->getOpcode();
----------------
ASDenysPetrov wrote:
>
Yep, good point.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:46-49
+ if (Assumption) {
+ return assumeSymRel(State, SIE, BO_NE, Zero);
+ }
+ return assumeSymRel(State, SIE, BO_EQ, Zero);
----------------
ASDenysPetrov wrote:
>
>
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112296/new/
https://reviews.llvm.org/D112296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
