[PATCH] D99374: [AsmParser][SystemZ][z/OS] Add support to AsmLexer to accept HLASM style integers

Abhina Sree via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 29 05:48:54 PDT 2021


abhina.sreeskantharajan added a comment.

I got some very minor nits about formatting, but overall it looks good



================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:323
 AsmToken AsmLexer::LexDigit() {
+  // HLASM-flavour decimal integer: [0-9]*
+  // FIXME: Later on, support for fb has to be added in
----------------
nit: I think we should stick to either HLASM-style or HLASM-flavour for consistency and not both.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:329
+    // They don't mean octal integers
+    unsigned Radix = doHexLookAhead(CurPtr, 10, /*LexMasmIntegers*/ false);
+    StringRef Result(TokStart, CurPtr - TokStart);
----------------
nit: comment style should be like the following to be clang-tidy friendly https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html


================
Comment at: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp:389
+  // StringRef AsmStr = "123";
+  //  Setup.
+  setupCallToAsmParser(AsmStr);
----------------
extra space here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99374



More information about the llvm-commits mailing list