[PATCH] D108825: [yaml2obj] Allow variable number of directories

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/COFFEmitter.cpp:462
+    for (uint32_t I = 0;
+         I < CP.Obj.OptionalHeader->Header.NumberOfRvaAndSize; ++I) {
+      const Optional<COFF::DataDirectory> *DataDirectories =
----------------
Nit: please remember to reformat modified/new code.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:1
+## Check that the default NumberOfRvaAndSize is as expected
+# RUN: yaml2obj --docnum=1 %s -o %t
----------------
Comments are full sentences and should end in a full stop (applies in tests as well as source files).

Same goes for each of your other new comments.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:2-3
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-readobj --file-headers %t | FileCheck %s
+# RUN: obj2yaml %t | FileCheck %s --check-prefix=ROUNDTRIP
+
----------------
jhenderson wrote:
> I'd rename the prefixes in this case to CHECK16 and ROUNDTRIP16, to show what value they are effectively testing.
Ping this comment?


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

https://reviews.llvm.org/D108825