[PATCH] D57321: Fix LexFloatLiteral Lexing

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:64
 
 /// LexFloatLiteral: [0-9]*[.][0-9]*([eE][+-]?[0-9]*)?
 ///
----------------
Probably should also fix this comment.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:152
+         (*CurPtr != 'e' && *CurPtr != 'E')) ||
+        (!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) &&
+         (*CurPtr == 'e' && *CurPtr == 'E') &&
----------------
'e' and 'E' are identifier characters, so some of the checks here are redundant.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:153
+        (!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) &&
+         (*CurPtr == 'e' && *CurPtr == 'E') &&
+         (isDigit(CurPtr[1]) ||
----------------
`*CurPtr == 'e' && *CurPtr == 'E'` is impossible.

We clearly need more test coverage given this issue wasn't caught by tests.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:338
+                                 (*CurPtr == '.' && CurPtr[1] == 'e')) &&
+                                isDigit(CurPtr[2])))) {
       ++CurPtr;
----------------
It's hard to follow this logic; when it's tangled together like this; does this accept `1.+1`?

Need more test coverage to catch cases like this.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:343
+      return LexFloatLiteral(false);
     }
 
----------------
If we conclude the suffix doesn't qualify as a float, we apparently treat it the suffix as an identifier; is that right?  Are the resulting diagnostics really going to be understandable?  (I guess "unexpected token in '.double' directive" is okay, although not great.)

Should we worry about binutils compatibility at all?  It apparently treats `1.e` as equivalent to `1.e0`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57321/new/

https://reviews.llvm.org/D57321