aaron.ballman added inline comments.
================ Comment at: clang/lib/Parse/ParseStmtAsm.cpp:937 + case AQ_goto: return "goto"; + case AQ_unspecified:; + } ---------------- nickdesaulniers wrote: > aaron.ballman wrote: > > This looks wrong to me -- it flows through to an unreachable despite being > > reachable. I think this should `return "unspecified";`. > This method is only called for printing; it seems weird to return > "unspecified" when it's kind of an invariant that that should never happen. > Maybe an assert here would be better? > > I've updated the implementation slightly, but leaving this comment thread > open for more feedback. An assert seems wrong to me -- the enumeration value is valid and the user has asked to print it. There's not much to assert on there. It's the caller of the method that should know whether it's violating an invariant to have that value in the first place. Also, people print from debuggers with relative frequency, so I'd personally avoid an assert in this case. ================ Comment at: clang/lib/Parse/ParseStmtAsm.cpp:684 + SkipUntil(tok::r_paren, StopAtSemi); + Diag(Tok.getLocation(), diag::err_asm_qualifier_ignored); + return true; ---------------- I think this will point to the right paren because of the preceding `SkipUntil` when it should point at the original token location. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75563/new/ https://reviews.llvm.org/D75563 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
