[PATCH] D17293: [MC] AsmLexer: add extensible identifier's character set support.

[Jim Grosbach via Phabricator via llvm-commits]

grosbach requested changes to this revision.
grosbach added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:238
+  bool getAllowAtInIdentifier() { return IsAllowedIDBodyChar('@'); }
+  void setAllowAtInIdentifier(bool v) { setIdentifierCharSet(v, "", "@"); }
+
----------------
With the generalization, these can go away entirely, yes? Replace the callsites w/ the new API.


================
Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:246
+  /// test whether the specified character can start an identifier
+  bool IsAllowedIDPrefixChar(char C) const {
+    return IdPrefixCharSet.test((unsigned char)C);
----------------
This should start with "is" not "Is" according the the coding guidelines.


================
Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:251
+  /// test whether the specified character can follow identifier start char
+  bool IsAllowedIDBodyChar(char C) const {
+    return IdBodyCharSet.test((unsigned char)C);
----------------
Ditto.


================
Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:258
+    return IsAllowedIDBodyChar(C) || IsAllowedIDPrefixChar(C);
+  }
 };
----------------
This feels really weird. Wouldn't any callsites want to be using one of the other two? They'll know their context. I don't see any invocations of this method in the patch. Why is it needed at all?


================
Comment at: lib/MC/MCParser/AsmLexer.cpp:576
+  }
+
   switch (CurChar) {
----------------
Can you elaborate on this bit? Not sure I follow why this is so much more logic than previously.


================
Comment at: lib/MC/MCParser/MCAsmLexer.cpp:37
+                                      StringRef PfxCharSet,
+                                      StringRef BodyCharSet) {
+  if (Value) {
----------------
Given the bimodal behaviour based on Value, this should probably just be two functions.


Repository:
  rL LLVM

https://reviews.llvm.org/D17293