[PATCH] D149439: [yaml2obj] Add support fot structured section data.

Tue Jul 4 00:07:45 PDT 2023

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

In D149439#4469654 <https://reviews.llvm.org/D149439#4469654>, @jacek wrote:

> Updated with missing tests. I also noticed that using uint32_t instead of int32_t would be better, so that we can cover call values with 0x constants.

Is there perhaps an argument for supporting both? `uint32_t` would certainly be my first choice, so I'm happy with just that, but if there's a case where the data might typically represent a negative value, it might make sense to support both. If there isn't a use-case though, it can be deferred until there is.

Anyway, LGTM in its current state.

================
Comment at: llvm/lib/ObjectYAML/COFFYAML.cpp:552
+    IO &IO, COFFYAML::SectionDataEntry &E) {
+  IO.mapOptional("Uint32", E.Uint32);
+  IO.mapOptional("Binary", E.Binary);
----------------
No more than food-for-thought, but should it be `UInt32` (capitalized "i"), since it's an abbreviation of "unsigned int"? I don't have a strong opinion, so am happy with whatevere your preference is.

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

https://reviews.llvm.org/D149439