[PATCH] D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2302
   std::string OpcodeStr = IDVal.lower();
   ParseInstructionInfo IInfo(Info.AsmRewrites);
+
----------------
Maybe those two lines should move into parseAndMatchAndEmitTargetInstruction, they are identical between the two callers.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2310
+    ParseInstructionInfo &IInfo, ParseStatementInfo &Info, StringRef Name,
+    AsmToken Token, SMLoc Loc) {
+
----------------
Can you rename the parameters so that you don't need to change the code below just because the names are different?   Ideally, the rest of the function should have an empty diff to the current source.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2324
         OS << ", ";
-      Info.ParsedOperands[i]->print(OS);
+      Info.ParsedOperands[I]->print(OS);
     }
----------------
Here's some more spurious differences that should be removed.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1393
       }
     }
     if (getLexer().isNot(AsmToken::EndOfStatement)) {
----------------
I suggested to move the HLASM remark handling here (*before* the EndOfStatement check), I still think this would be a better place.  Is there any reason why this doesn't work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98276