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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Sorry, I looked through this a while ago but forgot to hit "Submit" :(

This is looking pretty good. Only the test coverage / LengthOf==1 questions below left really.

It's weird that both parsing and emission are in the same (giant) class, but that's not new at least.



================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmParser.h:173
 
+  virtual bool LookupFieldOffset(StringRef Base, StringRef Member,
+                                 unsigned &Offset) {
----------------
epastor wrote:
> thakis wrote:
> > nit: LookUpFieldOffset. "Lookup" is a noun, "look up" is the verb.
> Changed. However... there are several functions in other files that use "lookup" rather than "lookUp" in a verb context. I'd been trying to match those.
Ah, local consistency is a good reason. No need to change it next time when I say something like that (but no need to change it back either)


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:320
+  size_t LengthOf = 0;
+  size_t Type = 0;
+  FieldInitializer Contents;
----------------
could you add comments what Type and LengthOf mean? Offset and SizeOf are self-explanatory, I can guess at LengthOf, but Type makes me come up empty.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3324
+    else
+      Struct.Size += Field.SizeOf;
+
----------------
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?)


================
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");
----------------
We're in a LengthOf == 1 block. How could we hit this? Does anything in here modify Field?




================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3624
+    } else if (parseOptionalAngleBracketOpen()) {
+      if (Field.LengthOf == 1)
+        return Error(Loc, "Cannot initialize scalar field with array value");
----------------
We're in a LengthOf == 1 block. How could we hit this? Does anything in here modify Field?


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3680
+    } else {
+      // Empty angle-bracket initializer
+      if (parseToken(AsmToken::LessGreater))
----------------
What if it's something else? This is user-controlled and could have arbitrary contents, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75306