rsmith added inline comments.
================
Comment at: include/clang/AST/Stmt.h:1008
+ (*iterator_adaptor_base::I)->getStmtClass() <= lastExprConstant);
+ return *reinterpret_cast<T **>(iterator_adaptor_base::I);
}
----------------
Ugh. This cast violates strict aliasing, and even in the absence of strict
aliasing won't work unless the `Stmt` base class is at offset 0 in `T`. The
preceding assert is also wrong, as it's asserting that `*I` is an `Expr`, not
that it's a `T`.
After r352925, you can use `CastIterator<AddrLabelExpr>` instead; that should
substantially reduce the churn in this patch.
================
Comment at: include/clang/AST/Stmt.h:2842
+
+ bool isGCCAsmGoto() const {
+ return NumLabels > 0;
----------------
This is already inside a class `GCCAsmStmt`; the `GCC` here seems unnecessary.
================
Comment at: include/clang/AST/Stmt.h:2860
+ StringRef getLabelName(unsigned i) const;
+ Expr *getExpr(unsigned i) const;
+ using labels_iterator = ExprIterator<AddrLabelExpr>;
----------------
Please give this a better name; `getExpr` is pretty meaningless.
================
Comment at: lib/Analysis/CFG.cpp:1466
+ LabelMapTy::iterator LI = LabelMap.find(E->getLabel());
+ if (LI == LabelMap.end()) continue;
+ JT = LI->second;
----------------
If this happens for every iteration of the loop, `VD` is used uninitialized
after the loop.
================
Comment at: lib/Analysis/CFG.cpp:1472
+ JT.scopePosition);
+ VD = prependAutomaticObjScopeEndWithTerminator(B, I->scopePosition,
+ JT.scopePosition);
----------------
It is extremely suspicious to be overwriting the value of `VD` on each
iteration of the loop like this. If you are assuming that the same value will
be returned every time, please include an `assert` of that.
================
Comment at: lib/Analysis/CFG.cpp:2126
+ if (cast<GCCAsmStmt>(S)->isGCCAsmGoto())
+ return VisitGCCAsmStmt(cast<GCCAsmStmt>(S));
+ return VisitStmt(S, asc);
----------------
This is not an appropriate function name for a function that is only called on
`asm goto`. Given the convention here is to match the `Stmt` class hierarchy
with the `Visit` functions, the branch on `asm goto` should be in the callee.
================
Comment at: lib/CodeGen/CGStmt.cpp:1884
+template <typename T>
+void CodeGenFunction::UpdateCallInst(
+ T &Result, bool HasSideEffect, bool ReadOnly, bool ReadNone,
----------------
This is not a reasonable function name for a function that is specific to
`AsmStmt`s. Please also make this a (static) non-member function, since it
cannot be called outside this source file.
================
Comment at: lib/CodeGen/CGStmt.cpp:2238-2245
+ UpdateCallInst<llvm::CallBrInst>(*Result, HasSideEffect, ReadOnly,
+ ReadNone, S, ResultRegTypes, RegResults);
+ EmitBlock(Fallthrough);
} else {
- for (unsigned i = 0, e = ResultRegTypes.size(); i != e; ++i) {
- llvm::Value *Tmp = Builder.CreateExtractValue(Result, i, "asmresult");
- RegResults.push_back(Tmp);
- }
+ llvm::CallInst *Result =
+ Builder.CreateCall(IA, Args, getBundlesForFunclet(IA));
+ UpdateCallInst<llvm::CallInst>(*Result, HasSideEffect, ReadOnly, ReadNone,
----------------
No need to explicitly specify the template argument here; it's deducible.
================
Comment at: lib/Parse/ParseStmtAsm.cpp:833-837
+ if (Tok.isNot(tok::identifier)) {
+ Diag(Tok, diag::err_expected) << tok::identifier;
+ SkipUntil(tok::r_paren, StopAtSemi);
+ return StmtError();
+ }
----------------
This should be inside the loop below.
================
Comment at: lib/Parse/ParseStmtAsm.cpp:839
+ if (Tok.isNot(tok::r_paren)) {
+ while (1) {
+ LabelDecl *LD = Actions.LookupOrCreateLabel(Tok.getIdentifierInfo(),
----------------
`while (true)`
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
