[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