[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1468-1477
   std::optional<RangeSet> getRangeForNegatedSymSym(const SymSymExpr *SSE) {
     return getRangeForNegatedExpr(
         [SSE, State = this->State]() -> SymbolRef {
           if (SSE->getOpcode() == BO_Sub)
             return State->getSymbolManager().getSymSymExpr(
-                SSE->getRHS(), BO_Sub, SSE->getLHS(), SSE->getType());
+                SSE->getLHS(), BO_Sub, SSE->getRHS(), SSE->getType());
           return nullptr;
----------------
tahonermann wrote:
> steakhal wrote:
> > Now this is within my realm.
> > This code applies the following transformation: `-(X - Y)  =>  (Y - X)` .
> > Consequently, this is intentional.
> Ideally, this change would have tripped up a test. Do we have tests that attempt to verify source locations such that one could be added? I know testing source locations is difficult and tedious.
> Ideally, this change would have tripped up a test.
I think it did. I had a quick look at the premerge test bot on buildkite, and there is a test failure likely related to this hunk. See clang/test/Analysis/ptr-arith.c
Check [[ https://buildkite.com/llvm-project/premerge-checks/builds/171979#018a0995-4c0b-4703-aef0-75e0a8def7c7 | buildkite ]].


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1529-1536
       // If ranges were not previously found,
       // try to find a reversed expression (y > x).
       if (!QueriedRangeSet) {
         const BinaryOperatorKind ROP =
             BinaryOperator::reverseComparisonOp(QueriedOP);
-        SymSym = SymMgr.getSymSymExpr(RHS, ROP, LHS, T);
+        SymSym = SymMgr.getSymSymExpr(LHS, ROP, RHS, T);
         QueriedRangeSet = getConstraint(State, SymSym);
----------------
steakhal wrote:
> If you have recommendations on improving the comments of the surrounding context, let me know.
This breaks the test: clang/test/Analysis/constraint_manager_conditions.cpp
Check [[ https://buildkite.com/llvm-project/premerge-checks/builds/171979#018a0995-4c0b-4703-aef0-75e0a8def7c7 | buildkite ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285