[PATCH] D98519: [M68k] Add support for Motorola literal syntax to AsmParser

[Anirudh Prasad via Phabricator via llvm-commits]

anirudhp added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:36
   AllowAtInIdentifier = !StringRef(MAI.getCommentString()).startswith("@");
+  LexMotorolaIntegers = MAI.shouldUseMotorolaIntegers();
 }
----------------
ricky26 wrote:
> anirudhp wrote:
> > If you're setting this in the constructor, I'm hoping you're aware that for any other invocations of the AsmParser instance, except from llvm-mc, the lexing of motorola integers will not work as "expected", until the `UseMotorolaIntegers` attribute is explicitly set in the Motorola's inherited version of `MCAsmInfo`
> > 
> > If the only intended invocation is from llvm-mc for now, then maybe it makes sense to remove the MCAsmInfo attribute completely, since you already declared `LexMotorolaIntegers` as `LexMotorolaIntegers = false` in MCAsmLexer.h.
> That's about the size of it. I kept it this way so that I can happily build tests for this piece of functionality (which affects more than just the experimental platform) without building for M68k. Then, when the M68k AsmParser implementation lands, it'll include the change to `M68kMCAsmInfo`.
> 
> I wanted to keep the `MCAsmInfo` change here to avoid bundling changes into the already quite large bootstrapping of the M68k assembler.
> 
> It feels a bit off to populate this in the constructor as I've done, so I'm very much open to alternatives. 
>I wanted to keep the MCAsmInfo change here to avoid bundling changes into the already quite large bootstrapping of the M68k assembler.

Hmm I see. I guess the only other suggestion I have in this case, is to have a small patch that just deals with setting the MCAsminfo changes, and the setting of the variable in the MCAsmLexer constructor, and have your big asm parser patch depend on that.

But at the same time that might be just increasing your work unnecessarily :), and if the particular attribute is going to be set soon in a forthcoming patch, then it should be fine to have your changes in this patch the way they are currently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98519