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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
