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

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

steakhal requested changes to this revision.
steakhal added subscribers: tbaeder, ABataev.
steakhal added a comment.
This revision now requires changes to proceed.

I think we have two 2 TPs, that were interesting to see. I've CCd the code owners of the affected parts to confirm this.

As a general note,this patch is not NFC, thus it's expected to demonstrate the behavior difference by a test.
Without tests, at least in the Static Analyzer, we generally don't accept patches.

---

I've seen a few similar patches from you @Manna and probably some related folks.
Could you please clarify the motivation and the expectations? Or feel free to point me to the Discuss post around the subject if I happened to miss it.



================
Comment at: clang/lib/AST/Interp/InterpBlock.cpp:94-95
 
 DeadBlock::DeadBlock(DeadBlock *&Root, Block *Blk)
-    : Root(Root), B(Blk->Desc, Blk->IsStatic, Blk->IsExtern, /*isDead=*/true) {
+    : Root(Root), B(Blk->Desc, Blk->IsExtern, Blk->IsStatic, /*isDead=*/true) {
   // Add the block to the chain of dead blocks.
----------------
I think this is a TP.
I find this unfortunate, that while all the ˙Block` ctors accepts the parameters in the order how the backing fields are defined - except for the `isDead` ctor overload, where the `IsExtern` and `IsStatic`.

To address this, I'd recommend not swapping the parameters at the callsite, but rather fix the ctor overload to expect the parameters in the right order as the rest of the ctors do. And of course, check all the callsites to this weird constructor to fix them as well.

WDYT @tbaeder


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:18098-18101
   case OMPC_allocate:
     Res = ActOnOpenMPAllocateClause(Data.DepModOrTailExpr, VarList, StartLoc,
-                                    LParenLoc, ColonLoc, EndLoc);
+                                    ColonLoc, LParenLoc, EndLoc);
     break;
----------------
Looks like another TP.
WDYT @ABataev


================
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;
----------------
Now this is within my realm.
This code applies the following transformation: `-(X - Y)  =>  (Y - X)` .
Consequently, this is intentional.


================
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);
----------------
If you have recommendations on improving the comments of the surrounding context, let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285