rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.
Looks good with a few largely-mechanical changes.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:30
"GNU-style inline assembly is disabled">;
-def err_asm_goto_not_supported_yet : Error<
- "'asm goto' constructs are not supported yet">;
+def err_asm_goto_can_not_have_output : Error<
+ "'asm goto' cannot have output constraints">;
----------------
Nit: `can_not` -> `cannot`
================
Comment at: lib/Parse/ParseStmtAsm.cpp:806
// Parse the asm-string list for clobbers if present.
- if (Tok.isNot(tok::r_paren)) {
+ if (!AteExtraColon && Tok.isNot(tok::r_paren) &&
+ isTokenStringLiteral()) {
----------------
Nit: the `Tok.isNot(tok::r_paren)` check here is redundant, since
`isTokenStringLiteral()` covers that check.
================
Comment at: lib/Parse/ParseStmtAsm.cpp:821
}
+ if (!isGotoAsm && Tok.isNot(tok::r_paren)) {
+ Diag(Tok, diag::err_expected) << tok::r_paren;
----------------
I think this should be:
```
if (!isGotoAsm && (Tok.isNot(tok::r_paren) || AteExtraColon)) {
```
otherwise we'll accept a spurious extra colon split from a `::` at the end of a
non-`goto` `asm`. Eg, I think in C++:
```
void f() { asm("" : : :: ); }
```
... would be incorrectly accepted, because we'll parse the final `::` as
introducing the clobbers list, consume the `::` token, pass this check because
the next token is now `)`, and never notice we didn't use the second half of
the `::` token.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-671
+ if (NS->getIdentifier(i))
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+ Exprs[i]));
+ for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; ++i)
+ if (NS->getIdentifier(i))
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+ Exprs[i]));
----------------
jyu2 wrote:
> rsmith wrote:
> > This is still exposing / relying on an implementation detail of
> > `GCCAsmStmt`. Please replace `getIdentifier` with `getOuptutIdentifier` /
> > `getInputIdentifier` / `getLabelIdentifier`, adjust the indexing to match,
> > and remove `GCCAsmStmt::getIdentifier`. Or alternatively, iterate over
> > `Names` here rather than walking the parts of the `GCCAsmStmt`.
> Changed. Like this!!!
This is really neat, thanks!
================
Comment at: lib/Sema/SemaStmtAsm.cpp:672
+ // Sort NamedOperandList.
+ stable_sort(NamedOperandList.begin(), NamedOperandList.end(),
+ [](const NamedOperand &LHS, const NamedOperand &RHS) {
----------------
Nit: explicitly spell this as `std::stable_sort` instead of relying on ADL to
find it from the `std::` in the type of `NamedOperand`.
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
