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

[Eric Astor via Phabricator via llvm-commits]

epastor marked 21 inline comments as done.
epastor added inline comments.


================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmParser.h:173
 
+  virtual bool LookupFieldOffset(StringRef Base, StringRef Member,
+                                 unsigned &Offset) {
----------------
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.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:123
+  size_t Size = 0;
+  std::vector<FieldInfo> Fields;
+  StringMap<size_t> FieldsByName;
----------------
thakis wrote:
> Should this be a SmallVector?
> 
> (The clang approach would probably to allocate field infos right after the struct info so that it's in one slab, but probably not worth it here.)
It can't be a SmallVector, since FieldInfo is an incomplete type.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:211
+    case FT_STRUCT:
+      llvm_unreachable("Cannot initialize a struct field with integral values");
+      break;
----------------
thakis wrote:
> s/integral values/struct values/
> 
> 
> ...but why pass in a FieldType at all if only one value is valid? Why not just
> 
> ```
> FieldInitializer(SmallVector<const MCExpr *, 1> &&Values) : FT(FT_INTEGRAL) {
>   new (&IntInfo) IntFieldInfo(Values);
> }
> ```
> 
> ?
Good point; fixed!


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:357
+    if (Size % Alignment != 0) {
+      Size += Alignment - (Size % Alignment);
+    }
----------------
thakis wrote:
> can you use llvm::alignTo() for this?
That is EXTREMELY useful, and I didn't realize it existed! Thanks!


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3527
+
+    return false;
+  }
----------------
thakis wrote:
> This is identical to the else you added in the previous function as far as I can tell. Can you change this to be less copy-pasty? (extract function, or make one of the two dispatch to the other, or what have you)
Thanks for highlighting this; I'd missed an easy way to clean up the repetition.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3540
+      Tok.is(AsmToken::LessLess) || Tok.is(AsmToken::LessGreater)) {
+    if (Field.LengthOf == 1 && Field.Type > 1)
+      return Error(Loc, "Cannot initialize scalar field with array value");
----------------
thakis wrote:
> this is very nested. maybe try this structure:
> 
>   if lcurly
>     return parseFieldInitializerCurly()
>   if less
>     return parseFieldInitializeLess
>   if ...
> 
> i.e. add helper functions, with comments. If you find yourself at nesting depth 6, that's often a sign that extracting a function might be a good idea. I find "every part of a function should be at the same level of abstraction" a good rule of thumb fairly often.
Thank you very much for highlighting this. I'd been very aware of it as a problem at the time, but failed to find a good way to clean it up. With your prompting and fresh eyes, I've been able to find a much cleaner approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75306