[PATCH] D112296: [Analyzer][solver] Handle adjustments in constraint assignor remainder

[Denys Petrov via Phabricator via cfe-commits]

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