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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
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.
----------------
steakhal wrote:
> 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
I would be surprised if it makes sense for both `IsExtern` and `IsStatic` to be true. Perhaps `Block` could be modified to hold a single data member of type `StorageClass` that is asserted to be one of `SC_None`, `SC_Extern`, or `SC_Static`?


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285