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

[Jennifer Yu via Phabricator via cfe-commits]

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?
Thanks.


================
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.
Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56571/new/

https://reviews.llvm.org/D56571