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

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu accepted this revision.
myhsu added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:36
   AllowAtInIdentifier = !StringRef(MAI.getCommentString()).startswith("@");
+  LexMotorolaIntegers = MAI.shouldUseMotorolaIntegers();
 }
----------------
anirudhp wrote:
> 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.
I ca definitely foresee a more modular changes if we split the MCAsmInfo attribute and the populating `LexMotorolaIntegers` in constructor into a separate patch. But to be honest I feel like this is a really minor software engineering issue.
It will only concern me if reverting any of these patches causes large scale disaster. But  either we split this patch or not, M68k will be the only one get affected. So I'm fine with the current arrangement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98519