[PATCH] D97748: [SystemZ][z/OS] Add support to validate a HLASM Label.

[Anirudh Prasad via Phabricator via llvm-commits]

anirudhp added a comment.

Thank you reviewers for your feedback! I will address them shortly.

An additional point:
I checked the V1R6 Reference manual again and I seem to have forgotten to include a check for "National Characters" ($,@ and #). I would like to remedy this by adding two private inline functions `isHLASMAlpha` and `isHLASMAlnum` which encapsulates the checks for what is an "HLASM" alphabet, and use them in the overridden `isLabel` routine. Is this acceptable?



================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1605
+    // Rules:
+    // 1. Starts with an alphabetic character, followed by 62-alphanumberic
+    // characters
----------------
hubert.reinterpretcast wrote:
> Please fix the spelling for "alphanumeric". Also, I don't think the hyphen should be used here. If the "62" is a reference to the possible length, then the sentence is missing "up to".
> 
> Also, by "alphabetic", you mean (aside from `_`) 'considered alphabetic in the "C" locale'?
Yes. By "alphabetic", aside from "_" it is what is considered alphabetic in the "C" locale (a character in the range 'a' to 'z' or 'A' to 'Z')



================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1621
+    // '_' is treated as an alphabetic character.
+    if (!isalpha(RawLabel[0]) && RawLabel[0] != '_')
+      return !Error(Loc, "HLASM Label has to start with an alphabetic "
----------------
hubert.reinterpretcast wrote:
> Is this supposed to be a call to the locale-sensitive check? LLVM has an `isAlpha` function.
As mentioned above, the intent of the call to `isalpha` is to check whether the character is alphabetic according to the c-locale.

If it is preferred to use the isAlpha function provided in `StringExtras.h` I can use it.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1629
+    for (unsigned I = 1; I < RawLabel.size(); I++)
+      if (!isalnum(RawLabel[I]) && RawLabel[I] != '_')
+        return !Error(Loc, "HLASM Label has to be alphanumeric");
----------------
hubert.reinterpretcast wrote:
> See my comment about `isalpha` versus `isAlpha`.
Same comment as above. If it is preferred to use the LLVM `isAlnum` I can use that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97748