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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added inline comments.


================
Comment at: lib/AST/StmtProfile.cpp:324
     VisitStringLiteral(S->getClobberStringLiteral(I));
+  ID.AddInteger(S->getNumLabels());
 }
----------------
rsmith wrote:
> Don't we also need to profile the labels themselves?
How this can be lost? :-( 

Changed.


================
Comment at: lib/Analysis/CFG.cpp:1472
+          continue;
+        addSuccessor(B, JT.block);
+      }
----------------
rsmith wrote:
> It looks like we're not modeling the destructors for local variables as running on these CFG edges any more. Is that intentional?
Yes.  That it is my intention.  Since clean up code are not generated for asm-goto.


================
Comment at: lib/Sema/JumpDiagnostics.cpp:810
     return;
-  S.Diag(Jump->getGotoLoc(), diag::err_indirect_goto_in_protected_scope);
-  S.Diag(Target->getStmt()->getIdentLoc(), diag::note_indirect_goto_target);
+  bool IsAsmGoto = dyn_cast<GCCAsmStmt>(Jump) != nullptr;
+  S.Diag(Jump->getBeginLoc(), diag::err_indirect_goto_in_protected_scope)
----------------
rsmith wrote:
> `isa<GCCAsmStmt>(Jump)`
Yes!! changed. 


================
Comment at: lib/Sema/JumpDiagnostics.cpp:857
   if (!Diagnosed && !ToScopesCXX98Compat.empty()) {
-    S.Diag(Jump->getGotoLoc(),
-           diag::warn_cxx98_compat_indirect_goto_in_protected_scope);
-    S.Diag(Target->getStmt()->getIdentLoc(), diag::note_indirect_goto_target);
+    bool IsAsmGoto = dyn_cast<GCCAsmStmt>(Jump) != nullptr;
+    S.Diag(Jump->getBeginLoc(),
----------------
rsmith wrote:
> Use `isa` here too.
changed


================
Comment at: lib/Sema/SemaStmtAsm.cpp:656-657
+  // Check for duplicate asm operand name between input, output and label lists.
+  typedef std::pair<StringRef , Expr *> MyItemType;
+  SmallVector<MyItemType, 4> MyList;
+  for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
----------------
rsmith wrote:
> `MyItemType` and `MyList` aren't useful names for a reader of this code. Please use a better name. Maybe `NamedOperand` and `NamedOperandList`?
Changed.


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

https://reviews.llvm.org/D56571