[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

Jennifer Yu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 4 11:13:49 PST 2019

jyu2 marked 12 inline comments as done.
jyu2 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);
rsmith wrote:
> 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.
Hi Richard,
Thanks for explain for this. And thank you so much for proving CastIterator!!  I change the code to use this.

Comment at: include/clang/AST/Stmt.h:2842
+  bool isGCCAsmGoto() const {
+    return NumLabels > 0;
rsmith wrote:
> This is already inside a class `GCCAsmStmt`; the `GCC` here seems unnecessary.
Remove GCC

Comment at: include/clang/AST/Stmt.h:2860
+  StringRef getLabelName(unsigned i) const;
+  Expr *getExpr(unsigned i) const;
+  using labels_iterator = ExprIterator<AddrLabelExpr>;
rsmith wrote:
> Please give this a better name; `getExpr` is pretty meaningless.
should I change to getOperandExpr?

Comment at: lib/Analysis/CFG.cpp:1474
+          appendScopeBegin(JT.block, VD, G);
+          addSuccessor(B, JT.block);
+        }
jyu2 wrote:
> efriedma wrote:
> > Please don't copy-paste code.
> :-(  changed
I did this during in VisitGCCAsmStmt by not call addSuccessor when we need backpatch the the labels.

Comment at: lib/Analysis/CFG.cpp:1466
+          LabelMapTy::iterator LI = LabelMap.find(E->getLabel());
+          if (LI == LabelMap.end()) continue;
+          JT = LI->second;
rsmith wrote:
> If this happens for every iteration of the loop, `VD` is used uninitialized after the loop.
I am totally wrong.  Changed

Comment at: lib/Parse/ParseStmtAsm.cpp:839
+    if (Tok.isNot(tok::r_paren)) {
+      while (1) {
+        LabelDecl *LD = Actions.LookupOrCreateLabel(Tok.getIdentifierInfo(),
rsmith wrote:
> `while (true)`
Yes.  Changed.



More information about the cfe-commits mailing list