[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/COFFYAML.cpp:565-566
+  IO.mapOptional("Size", S.Size, support::ulittle32_t(sizeof(S)));
+  if (S.Size < sizeof(S.Size)) {
+    IO.setError("Size must be at least " + Twine(sizeof(S.Size)));
+    return;
----------------
Are you sure that wants to be `sizeof(S.Size)` in these two lines? Would it be worth giving `S` a more descriptive name?


================
Comment at: llvm/test/tools/yaml2obj/COFF/load-config-empty.yaml:1
+# Create load config with all fields using default values.
+
----------------
Nit: in newer tools tests, we tend to use `##` for comments, to help distinguish them from RUN/CHECK lines etc. Same throughout.


================
Comment at: llvm/test/tools/yaml2obj/COFF/load-config-large.yaml:1
+# Create laod config larger than coff_load_configuration32 struct.
+
----------------
I'm beginning to think it might be worth grouping all these tests into a single test file, with multiple YAMLs (using yaml2obj --docnum to pick the relevant YAML).  This is because the behaviour is all closely related to each other, and having the tests in the same file would therefore make it easier to find all tests to do with a particular feature.


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

https://reviews.llvm.org/D149440