[PATCH] D89729: [ms] [llvm-ml] Enable support for MASM-style macro procedures

[Eric Astor via Phabricator via llvm-commits]

epastor added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:443
+  // Number of locals defined.
+  uint16_t LocalCounter = 0;
+
----------------
thakis wrote:
> why not just `unsigned`?
Attempting to match ML.EXE; it uses an explicitly 16-bit counter to label its local symbols, wrapping (and thus failing with duplicate symbol definitions) if more than that many local symbols are defined.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:2259
-    case DK_NOALTMACRO:
-      return parseDirectiveAltmacro(IDVal);
     case DK_EXITM:
----------------
thakis wrote:
> did the altmacro call here disappear intentionally? probably, but delete parseDirectiveAltmacro()'s definition too then?
Yes, this is intentional. Done.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:2400
+    Lex();
+    return parseDirectiveMacro(IDVal, IDLoc);
   }
----------------
thakis wrote:
> why is this moving down here?
Because it isn't just moving; this section is used for MASM's infix directives, which includes MACRO. (e.g., you don't start a line with MACRO, you write <macroName> MACRO.)


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:2622
 
   // A macro without parameters is handled differently on Darwin:
   // gas accepts no arguments and does no substitutions
----------------
thakis wrote:
> comment is now outdated
Removed.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:2688
+      Pos += Argument.size();
+      if (Body[Pos] == '&') {
+        ++Pos;
----------------
thakis wrote:
> is this guaranteed to be in bounds?
... no. This was fixed later in the stack, and I forgot to backport it. Thanks for catching!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89729