[PATCH] D75306: [ms] [llvm-ml] Add initial MASM STRUCT/UNION support

[Eric Astor via Phabricator via llvm-commits]

epastor added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3324
+    else
+      Struct.Size += Field.SizeOf;
+
----------------
thakis wrote:
> This is again identical to the added block in the previous function; extract a addFieldToCurrentStruct(IdVal, Size) or addIntegralField (for consistency with the Real version below) function. (Is Name / NameLocnot used here in the StructInProgress case?)
Good point. Done.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3617
+    if (parseOptionalToken(AsmToken::LCurly)) {
+      if (Field.LengthOf == 1)
+        return Error(Loc, "Cannot initialize scalar field with array value");
----------------
thakis wrote:
> We're in a LengthOf == 1 block. How could we hit this? Does anything in here modify Field?
> 
> 
Sorry, copy/paste error. Fixed.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3680
+    } else {
+      // Empty angle-bracket initializer
+      if (parseToken(AsmToken::LessGreater))
----------------
thakis wrote:
> What if it's something else? This is user-controlled and could have arbitrary contents, no?
Yes, but in that case parseToken() will correctly fail due to not finding LessGreater... in which case we'll return "true" to pass the error upwards.

parseToken() is an imperative and returns true if it fails, unlike parseOptionalToken() which is an attempt and returns true if it succeeds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75306