[PATCH] D99889: [AsmParser][ms][X86] Fix possible misbehaviour in parsing of special tokens at start of string.

[Ricky Taylor via Phabricator via llvm-commits]

ricky26 added a comment.

I'm new to reviewing code here, so call me out if I mess up the etiquette. :)

Is it worth adding a non-backend-specific test in this patch too? It looks like this adds support for X86 identifiers starting with $ (I'm actually surprised this wasn't allowed before).

I have some reservations about having so many separate functions for specifying the identifier character set but to be honest I think it'll shake out if any further changes are needed in that area. (I guess you could probably add a flagset but that's probably premature at this stage.)

(Also, I landed my patch, so you're going to get a minor conflict when you rebase this. I apologise!)



================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:617-625
+  bool doesAllowQuestionTokenAtStartOfString() const {
+    return AllowQuestionTokenAtStartOfString;
+  }
+  bool doesAllowAtTokenAtStartOfString() const {
+    return AllowAtTokenAtStartOfString;
+  }
+  bool doesAllowDollarTokenAtStartOfString() const {
----------------
It might be better to name these `...AtStartOfIdentifier` (the previous nomenclature was `...AtNameStart`, that works too).

Also nitpick: I would consider dropping `Token` from the names or using `Symbol` since I'd argue that these symbols never become tokens in this case. That is super pedantic though. :)


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:146
 
-/// LexIdentifier: [a-zA-Z_.][a-zA-Z0-9_$.@#?]*
+/// LexIdentifier: [a-zA-Z_.?][a-zA-Z0-9_$.@#?]*
 static bool isIdentifierChar(char C, bool AllowAt, bool AllowHash) {
----------------
It's a bit awkward since this is a helper used elsewhere but if the comment represents all possible identifiers, then I think it needs `$` & `@` in the first character.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99889