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

[Eric Astor via Phabricator via llvm-commits]

epastor marked an inline comment as done.
epastor added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:167
+  FieldType FT;
+  union {
+    IntFieldInfo IntInfo;
----------------
rnk wrote:
> I think this union is creating a fair amount of code complexity. The more clang-y code pattern would be to do something like:
> - Use a BumpPtrAllocator for memory allocation to make these all owned by the asm parser, there should be one already somewhere
> - Use raw pointers to point between these classes, avoids need for unique_ptr or other ownership management
> - Set up the usual class hierarchy:
>     class FieldInitializer { FieldType FT; ... };
>     class IntFieldInit : public FieldInitializer { ... };
>     class RealFieldInit : public FieldInitializer { ... };
>     class StructFieldInit : public FieldInitializer { ... };
> 
> Then the parseFieldInitializer variants could return a nullable pointer to indicate parsing success or failure. WDYT?
> 
> The drawback of this approach is that objects in BumpPtrAllocators are not destroyed, so they cannot own external resources. Most vectors can just become ArrayRefs pointing to arrays in the same allocator. The main thing I see that this would affect is the StringMap for fields and APInt for int fields. So, I guess I'm not sure it's worth a big rewrite, but it's something to consider.
I agree it would might be worth the change - but the rewrite looks to be pretty big, and complicated due to several of the members in these objects. Let's move forward in a full review for now, and discuss the right answer as we go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75306