Page MenuHomePhabricator

[MC] Fix Various End Of Line Comment checkings
ClosedPublic

Authored by niravd on Oct 13 2016, 8:51 AM.

Details

Summary

Fix AsmParser lines to correctly handle end-of-line pre-processor
comments parsing when '#' is not the assembly line comment prefix.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd updated this revision to Diff 74532.Oct 13 2016, 8:51 AM
niravd retitled this revision from to [MC] Fix Various End Of Line Comment checkings.
niravd added a reviewer: rnk.
niravd updated this object.
niravd added a subscriber: llvm-commits.
rnk added inline comments.Oct 13 2016, 2:17 PM
lib/MC/MCParser/AsmParser.cpp
2973 ↗(On Diff #74532)

HasComma

lib/MC/MCParser/MCAsmParser.cpp
71 ↗(On Diff #74532)

I see a lot of usage of this that looks like:

bool HasComma;
parseOptionalToken(Comma, HasComma);
if (HasComma && ...)

Right now, this always returns false. Either the token is there and its not an error, or we consume it and its not an error. In that case, do you think we should indicate presence with the return type? It might be confusing because a boolean parse method return type usually indicates an error. WDYT? Usage would look like:

if (parseOptionalToken(AsmToken::Comma)) {
  if (parseIdentifier(...) || parseThing(...) || ...))
    return true / error / etc;
}
77 ↗(On Diff #74532)

This feels like it's below parseToken. Any reason to not keep using Lex?

niravd updated this revision to Diff 74691.Oct 14 2016, 8:08 AM

Modify parseOptionalToken

niravd added inline comments.Oct 17 2016, 7:34 AM
lib/MC/MCParser/MCAsmParser.cpp
71 ↗(On Diff #74532)

I had a similar thought myself but I was unhappy with the fact that the clean way to write this involves multiple conditional statements which duplicates the error suffix additions so I was pushing it off to another cleanup patch while I pondered a cleaner solution. In any case there's no real issue doing it here so I've folded it in.

77 ↗(On Diff #74532)

This is to correctly handle # preprocessor line comments as EndOfStatement for those cases that the lexer doesn't have the context.

rnk accepted this revision.Oct 21 2016, 4:52 PM
rnk edited edge metadata.

lgtm

include/llvm/MC/MCParser/MCAsmParser.h
183 ↗(On Diff #74691)

Please document the convention here, since it's not "return true on error".

This revision is now accepted and ready to land.Oct 21 2016, 4:52 PM
This revision was automatically updated to reflect the committed changes.