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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.
Herald added a reviewer: martong.


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


================
Comment at: lib/Analysis/CFG.cpp:1472
+          continue;
+        addSuccessor(B, JT.block);
+      }
----------------
It looks like we're not modeling the destructors for local variables as running on these CFG edges any more. Is that intentional?


================
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)
----------------
`isa<GCCAsmStmt>(Jump)`


================
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(),
----------------
Use `isa` here too.


================
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)
----------------
`MyItemType` and `MyList` aren't useful names for a reader of this code. Please use a better name. Maybe `NamedOperand` and `NamedOperandList`?


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

https://reviews.llvm.org/D56571