ASDenysPetrov accepted this revision.
ASDenysPetrov added inline comments.
This revision is now accepted and ready to land.
================
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);
----------------
martong wrote:
> 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.
>
> However, there is a subtle but very important difference.
Now I see the difference. OK, then. Your solution is sensible.
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
