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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1600
+bool SystemZAsmParser::isLabel(AsmToken &Token) {
+  if (getMAIAssemblerDialect() == AD_HLASM) {
+    // HLASM Labels are ordinary symbols
----------------
Checking the reverse condition and doing the quick exit first reduces the indentation level of almost the entire function body. Please consider it.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1601
+  if (getMAIAssemblerDialect() == AD_HLASM) {
+    // HLASM Labels are ordinary symbols
+    // An HLASM always starts at column 1.
----------------
Missing period at the end of the sentence.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1602
+    // HLASM Labels are ordinary symbols
+    // An HLASM always starts at column 1.
+    // An ordinary symbol syntax is laid out as follows:
----------------
Missing "label" after HLASM?


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1605
+    // Rules:
+    // 1. Starts with an alphabetic character, followed by 62-alphanumberic
+    // characters
----------------
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'?


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1607
+    // characters
+    //    Exception: "_" is treated as an alphabetical character
+    // 2. Maximum length of the symbol is 63 characters
----------------
Let's stick with "alphabetic"? Also, add the period to end the sentence.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1608
+    //    Exception: "_" is treated as an alphabetical character
+    // 2. Maximum length of the symbol is 63 characters
+    // 3. Labels are case insiginficant . Eg. "lab123", "LAB123", "lAb123" etc.
----------------
This is redundant with point 1 unless if some more subtle point is being made.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1609-1611
+    // 3. Labels are case insiginficant . Eg. "lab123", "LAB123", "lAb123" etc.
+    // are all treated as the same symbols. However the processing for the case
+    // will not be done in this function.
----------------
Please fix various typos, missing commas, and formatting issues.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1614
+    SMLoc Loc = Token.getLoc();
+
+    // An HLASM label cannot exceed greater than 63 characters.
----------------
It might not hurt to check that the string isn't empty somehow.


================
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 "
----------------
Is this supposed to be a call to the locale-sensitive check? LLVM has an `isAlpha` function.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1626
+    // Now, we've established that the length is valid
+    // and the first character is an alphabet.
+    // Check whether remaining string is alphanumeric.
----------------
s/an alphabet/alphabetic/;


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1628
+    // Check whether remaining string is alphanumeric.
+    for (unsigned I = 1; I < RawLabel.size(); I++)
+      if (!isalnum(RawLabel[I]) && RawLabel[I] != '_')
----------------
Minor nit: Use prefix `++`.


================
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");
----------------
See my comment about `isalpha` versus `isAlpha`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97748