rsmith added inline comments.
================
Comment at: lib/AST/Stmt.cpp:457-460
this->NumOutputs = NumOutputs;
this->NumInputs = NumInputs;
this->NumClobbers = NumClobbers;
+ this->NumLabels = NumLabels;
----------------
Please assert that you don't have both outputs and labels here. (If you did,
you would assign them the same slots within `Exprs`.)
You also seem to be missing `Sema` enforcement of the rule that you cannot have
both outputs and labels. (If you want to actually support that as an
intentional extension to the GCC functionality, you should change the
implementation of `GCCAsmStmt` to use different slots for outputs and labels,
add some tests, and add a `-Wgcc-compat` warning for that case. Seems better to
just add an error for it for now.)
================
Comment at: lib/Analysis/CFG.cpp:1445-1458
+ LabelMapTy::iterator LI = LabelMap.find(G->getLabel());;
+ // If there is no target for the goto, then we are looking at an
+ // incomplete AST. Handle this by not registering a successor.
+ if (LI == LabelMap.end()) continue;
- JumpTarget JT = LI->second;
- prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
- JT.scopePosition);
- prependAutomaticObjDtorsWithTerminator(B, I->scopePosition,
- JT.scopePosition);
- const VarDecl *VD = prependAutomaticObjScopeEndWithTerminator(
- B, I->scopePosition, JT.scopePosition);
- appendScopeBegin(JT.block, VD, G);
- addSuccessor(B, JT.block);
+ JumpTarget JT = LI->second;
+ prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
----------------
Please factor out this duplicated code into a local lambda rather than
copy-pasting it.
================
Comment at: lib/Analysis/CFG.cpp:1461
+ if (auto *G = dyn_cast<GCCAsmStmt>(B->getTerminator())) {
+ if (G->isAsmGoto()) {
+ for (auto *E : G->labels()) {
----------------
This check is redundant; there will be no labels if it's not an `asm goto` so
you can just perform the below loop unconditionally.
================
Comment at: lib/Analysis/CFG.cpp:3139-3141
+ // We will need to backpatch this block later.
+ BackpatchBlocks.push_back(JumpSource(Block, ScopePos));
+ return Block;
----------------
Do we not need the handling for the other labels if we hit this condition for
one label?
================
Comment at: lib/Sema/SemaStmtAsm.cpp:656
+ // Check for duplicate asm operand name between input, output and label
lists.
+ typedef std::pair <StringRef , Expr *> MyItemType;
+ SmallVector<MyItemType, 4> MyList;
----------------
No space after `pair`, please.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-662
+ for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+ if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+ NS->getOperandExpr(i)));
----------------
Looping over all three lists of expressions here is effectively hardcoding an
implementation detail of `GCCAsmStmt` into this code, violating its
encapsulation. Instead, please write three loops (iterating over the inputs,
outputs, and labels), and remove `getOperandExpr` entirely.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:660
+ for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+ if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
----------------
Checking whether the name is empty is redundant: we should never create an
`IdentifierInfo` representing an empty string.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56571/new/
https://reviews.llvm.org/D56571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
