[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:6139
+
+  checkAndGenerateLOCDirectiveForInstruction(ParseHadError, Loc);
+
----------------
uweigand wrote:
> anirudhp wrote:
> > uweigand wrote:
> > > I guess now this doesn't really have to be a separate routine any more?
> > Yes, you're right. But since the rest of the function is essentially just `getTargetParser().ParseInstruction` and `getTargetParser().MatchAndEmitInstruction()`, I didn't want to decompose logic in `checkAndGenerateLOCDirectiveForInstruction` into the same function. I'd rather have it as a separate routine, since it does something different.
> My point was that it might be preferable to minimize changes to existing code.  Just split the old parseStatement in two parts.  Leave the second part (the new parseAndMatchAndEmitTargetInstruction) where it is above.  The diff should just show the end of one function and the header of the new function being inserted, the rest of the lines are unchanged.
Can you please move this new parseAndMatchAndEmitTargetInstruction routine up in the source file to immediately after AsmParser::parseStatement ?  That should make the diff smaller and easier to review.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1396
       SMLoc Loc = getLexer().getLoc();
-      return Error(Loc, "unexpected token in argument list");
+      if (isParsingHLASM()) {
+        // Under the HLASM variant, we could have the remark field
----------------
You can avoid the duplicate error message by moving the code in here up before the isNot(Asm::EndOfStatement) check into a separate
  if (isParsingHLASM() && getTok().is(AsmToken::Space))
block


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1406
+
+        // We've confirmed that there is a Remark field.
+        StringRef Remark(getLexer().LexUntilEndOfStatement());
----------------
Should be skip the leading space(s), or are they intentionally part of the "remark"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98276