[PATCH] D99277: [AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmLexer.h:151
 
+  void setAllowHashInIdentifier(bool V) { AllowHashInIdentifier = V; }
+
----------------
anirudhp wrote:
> nickdesaulniers wrote:
> > The only caller is a test?
> > 
> > I think if you add an `llvm/test/MC/SystemZ` style test, you'd find this code probably isn't working as expected.
> > 
> > It's curious to me why this differs from how `AllowAtInIdentifier` is set in the constructor for `AsmLexer`.
> >The only caller is a test?
> 
> For now, yes. There is another patch in waiting (see https://reviews.llvm.org/D98276). Ideally, the call to the setter would be set in the constructor for the `HLASMAsmParser` class)
> 
> >I think if you add an llvm/test/MC/SystemZ style test, you'd find this code probably isn't working as expected.
> 
> A bit of background: 
> I probably should have mentioned this in the description, but these changes would be used by the z/OS target for which we cannot currently generate code yet (work for this is ongoing). Hence, the non-existence of a lit test (which would make things very easy for these types of changes). 
> 
> Since the LLVM community prefers to have a test for any functional code changes, I came up with a unit test in the interim, since this would help "alleviate" some of the wait time (i.e. while we wait for the corresponding ObjectFile/Streamer support for the z/OS target and other patches that might be in review)
> 
> To answer your question:
> Yes, a test in llvm/test/MC/SystemZ would not work as "intended" because the call to the setter isn't made anywhere. To be more specific, for the s390x-ibm-linux triple, the behaviour would be "unexpected". For an s390x-none-zos triple, the test wouldn't even run because we don't have relevant ObjectFile/Streamer support yet (as mentioned above)
> 
> >It's curious to me why this differs from how AllowAtInIdentifier is set in the constructor for AsmLexer.
> 
> It doesn't differ in terms of the behaviour that it is exposing. I don't really see an issue with moving it to a constructor and providing with similar behaviour (i.e. is MAI's CommentString isn't # set it to true), but since a default value of `false` was provided at the time of declaration, I didn't see the need for it.
> 
> If it is preferred to move to the constructor, I don't mind doing so. 
Since it will be immediately used and having the utility functions in this patch seem more appropriate, looks good.


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

https://reviews.llvm.org/D99277