[PATCH] D99277: [AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmLexer.h:151
 
+  void setAllowHashInIdentifier(bool V) { AllowHashInIdentifier = V; }
+
----------------
The only caller is a test?

I think if you add an `llvm/test/MC/SystemZ` style test, you'd find this code probably isn't working as expected.

It's curious to me why this differs from how `AllowAtInIdentifier` is set in the constructor for `AsmLexer`.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:149
+  return isAlnum(C) || C == '_' || C == '$' || C == '.' ||
+         (C == '@' && AllowAt) || C == '?' || (C == '#' && AllowHash);
 }
----------------
If you flip the order of `AllowHash && C == '#'` that would be better, since `AllowHash` is mostly always false for the general case. Feel free to reorder `AllowAt` and move both of those to be the final checks.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:733
+          isIdentifierChar(CurChar, MAI.doesAllowAtInName(),
+                           AllowHashInIdentifier))
         return LexIdentifier();
----------------
do we need a similar method to `MCAsmInfo::doesAllowAtInName`?  I wonder why `MCAsmInfo` and `MCAsmLexer` have their own fields for that? Seems like perhaps a (pre-existing) mistake.


================
Comment at: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp:98
-    // Lex initially to get the string.
-    Parser->getLexer().Lex();
   }
----------------
it's not clear to me how any of the changes to llvm/lib/ affect these changes to llvm/unittests/ in regards to explicitly calling `Lex`.


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

https://reviews.llvm.org/D99277