This is an archive of the discontinued LLVM Phabricator instance.

MIR Lexer: adopt the 'maybeLex...' pattern.
ClosedPublic

Authored by arphaman on Jun 29 2015, 3:02 PM.

Details

Summary

This patch refactors the MI lexer so that the lexing functions use the 'maybeLex...' pattern, where the lexing functions determine if they can lex the current token by themselves.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 28716.Jun 29 2015, 3:02 PM
arphaman retitled this revision from to MIR Lexer: adopt the 'maybeLex...' pattern..
arphaman updated this object.
arphaman edited the test plan for this revision. (Show Details)
arphaman added a reviewer: silvas.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: Unknown Object (MLST).
silvas edited edge metadata.Jun 29 2015, 6:36 PM

LGTM with a nit.

lib/CodeGen/MIRParser/MILexer.cpp
182

Even though I know this pattern returns a cursor, I found myself looking around to confirm that the auto was hiding a cursor. I would prefer to just say Cursor.

2015-06-29 18:36 GMT-07:00 Sean Silva <chisophugis@gmail.com>:

LGTM with a nit.

REPOSITORY

rL LLVM

Comment at: lib/CodeGen/MIRParser/MILexer.cpp:182
@@ -164,16 +181,3 @@

  • auto Char = C.peek();
  • if (isalpha(Char) || Char == '_')
  • return lexIdentifier(C, Token).remaining();
  • if (Char == '%') {
  • if (C.remaining().startswith("%bb."))
  • return lexMachineBasicBlock(C, Token, ErrorCallback).remaining();
  • return lexPercent(C, Token).remaining();
  • }
  • if (Char == '@')
  • return lexGlobalValue(C, Token).remaining();
  • if (isdigit(Char) || (Char == '-' && isdigit(C.peek(1))))
  • return lexIntegerLiteral(C, Token).remaining();
  • MIToken::TokenKind Kind = symbolToken(Char);
  • if (Kind != MIToken::Error)
  • return lexSymbol(C, Kind, Token).remaining();

+ if (auto R = maybeLexIdentifier(C, Token))

+ return R.remaining();

Even though I know this pattern returns a cursor, I found myself looking
around to confirm that the auto was hiding a cursor. I would prefer to
just say Cursor.

Thanks, I will commit this patch with 'auto' replaced by 'Cursor'.
Alex

http://reviews.llvm.org/D10817

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
This revision was automatically updated to reflect the committed changes.