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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:167
+  FieldType FT;
+  union {
+    IntFieldInfo IntInfo;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75306