[NFC][CLANG] Fix wrong orders of function arguments positions

[steakhal]

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.

clang/lib/AST/Interp/InterpBlock.cpp

94–95

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

clang/lib/Sema/SemaOpenMP.cpp

18098–18101

Looks like another TP.
WDYT @ABataev

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

1468–1477

Now this is within my realm.
This code applies the following transformation: -(X - Y) => (Y - X) .
Consequently, this is intentional.

1529–1536

If you have recommendations on improving the comments of the surrounding context, let me know.

[aaron.ballman]

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.

Intel has been addressing issues found by a static analysis tool and when we find the issue exists in community Clang, we address it upstream. Part of addressing the issue is figuring out whether the issue is actually a false positive or not; we try to limit the changes to only true positives (but some will sometimes slip through the cracks). I don't believe there's been a Discourse post on the topic as it's expected to be regular maintenance work instead of something special.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

18098–18101

Yes, this is the correct fix

[steakhal]

In D158285#4603477, @aaron.ballman wrote:

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.

Intel has been addressing issues found by a static analysis tool and when we find the issue exists in community Clang, we address it upstream. Part of addressing the issue is figuring out whether the issue is actually a false positive or not; we try to limit the changes to only true positives (but some will sometimes slip through the cracks). I don't believe there's been a Discourse post on the topic as it's expected to be regular maintenance work instead of something special.

Thanks for clarifying. It does resonate with me, and it is an important subject to work on. However, I sometimes feel that a little more effort in understanding the context where the suggestion is raised before review would sort out some of the FPs I usually encounter of such patches.
I find this important because it's difficult to allocate time for doing this.

To me, an important metric of a quality patch is if the title and the summary describe why we have this patch, and what we do about it. Maybe, if it matters, also describe what the author tried and didn't work and why.
An implicit benefit of this is that it helps the author to reflect and form a deeper understanding of the issue at hand (to be able to describe it).

In addition to this, if it's a behavioral change (like this), it's valuable to demonstrate the bug with a test case covering the changed parts.
I'm not enforcing this, as it doesn't always make sense. But thinking about it again helps to reflect. At worst, the result of this would materialize in "I don't have a test for this change because XYZ" in the summary of the revision.
Following these steps would improve review experience, which should also lead to happier patch authors in the form of quick and to-the-point review comments.

I hope I clarified my standpoint. @aaron.ballman

[tahonermann]

To me, an important metric of a quality patch is if the title and the summary describe why we have this patch, and what we do about it.

@steakhal, I'll work with the people submitting these patches to add more context to the title, summary, and commit comments. I agree that would be helpful.

Ideally, we would copy the report from the static analysis tool, but we have been informed that we are not allowed to divulge the tool we are using, so we aren't permitted to do that.

[aaron.ballman]

In D158285#4603521, @steakhal wrote:

In D158285#4603477, @aaron.ballman wrote:

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.

Intel has been addressing issues found by a static analysis tool and when we find the issue exists in community Clang, we address it upstream. Part of addressing the issue is figuring out whether the issue is actually a false positive or not; we try to limit the changes to only true positives (but some will sometimes slip through the cracks). I don't believe there's been a Discourse post on the topic as it's expected to be regular maintenance work instead of something special.

Thanks for clarifying. It does resonate with me, and it is an important subject to work on. However, I sometimes feel that a little more effort in understanding the context where the suggestion is raised before review would sort out some of the FPs I usually encounter of such patches.
I find this important because it's difficult to allocate time for doing this.

To me, an important metric of a quality patch is if the title and the summary describe why we have this patch, and what we do about it. Maybe, if it matters, also describe what the author tried and didn't work and why.
An implicit benefit of this is that it helps the author to reflect and form a deeper understanding of the issue at hand (to be able to describe it).

In addition to this, if it's a behavioral change (like this), it's valuable to demonstrate the bug with a test case covering the changed parts.
I'm not enforcing this, as it doesn't always make sense. But thinking about it again helps to reflect. At worst, the result of this would materialize in "I don't have a test for this change because XYZ" in the summary of the revision.
Following these steps would improve review experience, which should also lead to happier patch authors in the form of quick and to-the-point review comments.

I hope I clarified my standpoint. @aaron.ballman

+1, I agree with the points you're raising. My only concern is that some of your points are easier to address for people who are familiar with the code base but harder for folks who are not as well-versed. e.g., I think test coverage is reasonable to ask for when there are functional changes, but we should be pragmatic if it's a real challenge to devise a test case that trips the exact conditions. We may need to help path authors out with some of this as part of the review process, but your point still stands that the patch authors should try to capture more of the bigger picture before putting the patch up. Thank you!

[tahonermann]

clang/lib/AST/Interp/InterpBlock.cpp

94–95

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?

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

1468–1477

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.

[steakhal]

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

1468–1477

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 buildkite.

1529–1536

This breaks the test: clang/test/Analysis/constraint_manager_conditions.cpp
Check buildkite.

clang/lib/AST/Interp/InterpBlock.cpp

bool Block::hasPointer(const Pointer *P) const {
   for (const Pointer *C = Pointers; C; C = C->Next) {
     if (C == P)
       return true;
   }
   return false;
 }
 #endif
 
 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.
   if (Root)
     Root->Prev = this;
 
   Next = Root;
   Prev = nullptr;
   Root = this;
 

clang/lib/Sema/SemaOpenMP.cpp

case OMPC_use_device_addr:
     Res = ActOnOpenMPUseDeviceAddrClause(VarList, Locs);
     break;
   case OMPC_is_device_ptr:
     Res = ActOnOpenMPIsDevicePtrClause(VarList, Locs);
     break;
   case OMPC_has_device_addr:
     Res = ActOnOpenMPHasDeviceAddrClause(VarList, Locs);
     break;
   case OMPC_allocate:
     Res = ActOnOpenMPAllocateClause(Data.DepModOrTailExpr, VarList, StartLoc,
-                                    LParenLoc, ColonLoc, EndLoc);
+                                    ColonLoc, LParenLoc, EndLoc);
     break;
   case OMPC_nontemporal:
     Res = ActOnOpenMPNontemporalClause(VarList, StartLoc, LParenLoc, EndLoc);
     break;
   case OMPC_inclusive:
     Res = ActOnOpenMPInclusiveClause(VarList, StartLoc, LParenLoc, EndLoc);
     break;
   case OMPC_exclusive:
     Res = ActOnOpenMPExclusiveClause(VarList, StartLoc, LParenLoc, EndLoc);

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

return getRangeForNegatedExpr(
         [USE]() -> SymbolRef {
           if (USE->getOpcode() == UO_Minus)
             return USE->getOperand();
           return nullptr;
         },
         USE->getType());
   }
 
   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;
         },
         SSE->getType());
   }
 
   std::optional<RangeSet> getRangeForNegatedSym(SymbolRef Sym) {
     return getRangeForNegatedExpr(
         [Sym, State = this->State]() {
           return State->getSymbolManager().getUnarySymExpr(Sym, UO_Minus,
                                                            Sym->getType());
         },
         Sym->getType());
std::optional<RangeSet> getRangeForComparisonSymbol(const SymSymExpr *SSE) {
     // We treat `UnknownX2` column separately at the end of the loop body.
     for (size_t i = 0; i < CmpOpTable.getCmpOpCount(); ++i) {
 
       // Let's find an expression e.g. (x < y).
       BinaryOperatorKind QueriedOP = OperatorRelationsTable::getOpFromIndex(i);
       const SymSymExpr *SymSym = SymMgr.getSymSymExpr(LHS, QueriedOP, RHS, T);
       const RangeSet *QueriedRangeSet = getConstraint(State, SymSym);
 
       // 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 (!QueriedRangeSet || QueriedRangeSet->isEmpty())
         continue;
 
       const llvm::APSInt *ConcreteValue = QueriedRangeSet->getConcreteValue();
       const bool isInFalseBranch =
           ConcreteValue ? (*ConcreteValue == 0) : false;