[PATCH] MC: AsmLexer: handle multi-character CommentStrings correctly

Saleem Abdulrasool compnerd at compnerd.org
Fri Jul 25 19:59:40 PDT 2014


The tests feel a bit insufficient for such a change.  You have a special case for Darwin yet no test for it?  Is there a set of existing test cases that cover the various other cases of string  comment indicators?

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:462
@@ -461,2 +461,3 @@
 bool AsmLexer::isAtStartOfComment(char Char) {
-  // FIXME: This won't work for multi-character comment indicators like "//".
+  if (strlen(MAI.getCommentString()) > 1)
+    return false;
----------------
Isn't this not entirely true?  If you have '//' as a comment character, and you are at '/', its unclear if this is a comment starter or not.  I think that we should just drop this interface and take a const char * instead.

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:469
@@ +468,3 @@
+  const char *CommentString = MAI.getCommentString();
+  if (strlen(CommentString) > 1 &&
+      // FIXME: special case for the bogus comment string in X86MCAsmInfoDarwin
----------------
I think that special casing the 1 character into the fast path makes sense here:

    if (CommentString[1] == '\0')
      return *Ptr == CommentString[0];

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:470
@@ +469,3 @@
+  if (strlen(CommentString) > 1 &&
+      // FIXME: special case for the bogus comment string in X86MCAsmInfoDarwin
+      CommentString[0] == '#' && CommentString[1] == '#') {
----------------
Why not fix this properly rather than work around it?

http://reviews.llvm.org/D4597






More information about the llvm-commits mailing list