[PATCH] D25013: [x86][inline-asm][avx512] allow swapping of '{k<num>}' & '{z}' marks

coby via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 18 01:40:37 PDT 2016

coby added inline comments.

Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1897-1899
+  if (!getLexer().is(AsmToken::RCurly))
+    return !Error(getLexer().getLoc(), "Expected } at this point");
+  Parser.Lex(); // Eat '}'
rnk wrote:
> rnk wrote:
> > This can be simplified to:
> >   if (parseToken(AsmToken::RCurly, "expected } at this point"))
> >     return true;
> Any reason not to do this?
One would be conformity, the second will apply to readability and the other is a somewhat of a flavor issue:
1. While 'parseToken' might (or might not, haven't really checked) be considered as a common way of reporting errors - it is no where to be seen on the currently reviewed source file. hence, at least until global re-factorization is at hand - I would tend to stick to the old and (possibly) good.
2. the title 'parseToken' does brilliant job stating its role as a Parsing subroutine, but fails to reveal its part as an error-reporting mechanism.
3. imho, and as a side note, the entire error reporting handling is a bit of lacky, mainly the need to dictate an error message each time, but it is a subject for another discussion..



More information about the llvm-commits mailing list