[PATCH] D97703: [AsmParser][SystemZ][z/OS] Introducing HLASM Comment Syntax

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Can you add a test? It seems like you already have the `jo *-4` example so just a test that the following does what you expect would be enough for me:

  * comment
  jo *-4



================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:125
 
   /// This indicates the comment character used by the assembler.  Defaults to
   /// "#"
----------------
This seems to be stale? It seems like this should read "the comment string", unless the `strncmp` in `AsmLexer::isAtStartOfComment` is just dead code?


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:129
 
+  /// This indicates whether the comment character is only accepted as a comment
+  /// at the beginning of statements. Defaults to false.
----------------
Ditto here, the new option should apply for every valid `CommentString`, which appears to be any non-zero-length string (with some weird special case when the second character is `#`)


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:131
+  /// at the beginning of statements. Defaults to false.
+  bool RestrictCommentString = false;
+
----------------
I would prefer a name that describes how the comment string is restricted, maybe `RestrictCommentStringToStartOfStatement`? It is a bit long, but we already have e.g. `ZeroDirectiveSupportsNonZeroValue`, and these options are only used in a couple well-defined places where a descriptive name seems worth the extra characters.


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:667
+    return CommentString[0] == Ptr[0] &&
+           (!RestrictCommentString || IsAtStartOfStatement);
 
----------------
To implement the comment below, can this be replaced with another `if` at the start of the function:

```
if (RestrictCommentString && !IsAtStartOfStatement)
  return false;
```


================
Comment at: llvm/lib/MC/MCParser/AsmLexer.cpp:673
 
   return strncmp(Ptr, CommentString.data(), CommentString.size()) == 0;
 }
----------------
It may not be needed for HLASM, but if we are adding an option which appears to be orthogonal to `CommentString` then I think it ought to handle the non-single-character-string case. I.e. if `CommentString == "//"` then your current `RestrictCommentString` has an unexpected effect (i.e. it no longer applies).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97703