[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:6223
+  assert(!(ShouldParseAsMachineInstruction && ShouldParseAsLabel) &&
+         "Cannot parse first token  as both a label and an instruction");
+
----------------
Having two variables that are always the complement of one another seems odd, in particular because ShouldParseAsLabel doesn't appear to be used anywhere?


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:6244
+      return true;
+    return false;
+  }
----------------
Why not simply  "return parseAsMachineInstruction"?


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1403
+      Parser.Lex();
+
+      // If there is nothing after the space, then there is nothing to emit
----------------
This still looks odd to me.  What seems to be going on is that the LexUntilEndOfStatement lexes everything including the initial space.  The explicit Lex then lexes the EndOfStatement marker.

I believe that would be wrong: the EndOfStatement is already lexed below (last line of this function).  And the comment just immediately following here seems to imply that the "Lex" was intended to lex that initial *space*.  But for that to be true, wouldn't the Lex have to be done *before* the LexUntilEndOfStatement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98276