[PATCH] D149440: [yaml2obj] Add support for load config section data.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/COFFEmitter.cpp:606
+  OS.write(reinterpret_cast<const char *>(&S),
+           std::min(sizeof(S), size_t(S.Size)));
+  if (sizeof(S) < S.Size)
----------------
Having now reread this bit of code, I now see that `S.Size` is being cast to `size_t`, whereas I misread it originally as being a `sizeof` due ot the similarity it has with the `sizeof(S)` earlier in the line. Perhaps a `static_cast` would be more obvious?

I'm assuming that `S.Size` corresponds to the `Size` field in the YAML. Assuming that is the case, what happens if a load config member would be written AFTER the end of a truncated block (i.e. where `S.Size` is less than `sizeof(S)`)? Should this have a distinct test case?


================
Comment at: llvm/test/tools/yaml2obj/COFF/load-config-empty.yaml:32
+    StructuredData:
+      - LoadConfig:
+symbols: []
----------------
Is this equivalent to having a `Size: 0` case? If so, it might be worth being explicit about it. If not, it might be worth having that test case too.


================
Comment at: llvm/test/tools/yaml2obj/COFF/load-config32-large.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --hex-dump=.rdata %t | FileCheck %s
----------------
It would be useful having an explanatory comment at the top of each of these tests briefly outlining what they are specifically testing.


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

https://reviews.llvm.org/D149440