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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Just a few nits remaining, otherwise looks pretty good.



================
Comment at: llvm/lib/ObjectYAML/COFFEmitter.cpp:461
     }
-    for (const Optional<COFF::DataDirectory> &DD :
-         CP.Obj.OptionalHeader->DataDirectories) {
-      if (!DD.hasValue()) {
+    for (uint32_t NumDir = 0;
+         NumDir < CP.Obj.OptionalHeader->Header.NumberOfRvaAndSize; ++NumDir) {
----------------
Now that I see it written out like this, I find `NumDir` confusing as a name. Could you rename it to `I`, since it's a basic loop counter.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:1
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-readobj --file-headers %t | FileCheck %s
----------------
I'd add a comment (using `##` for the comment marker) before each run of yaml2obj to explain what is significant about each case (i.e. default/higher than normal/etc).


================
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
+
----------------
I'd rename the prefixes in this case to CHECK16 and ROUNDTRIP16, to show what value they are effectively testing.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:120-122
+# CHECK0:      NumberOfRvaAndSize: 0
+
+# ROUNDTRIP0:  NumberOfRvaAndSize: 0
----------------
Something needs to be added in these two cases to show that there's no DataDirectory dumped. It might be a CHECK-NOT or CHECK-EMPTY directive for the CHECK case, for example.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:129-130
+# RUN: yaml2obj --docnum=2 %s -o %t -DNUMRVA=6
+# RUN: llvm-readobj --file-headers %t | FileCheck %s --check-prefix=CHECK2
+# RUN: obj2yaml %t | FileCheck %s --check-prefix=ROUNDTRIP2
+
----------------
As above, use CHECK6 and ROUNDTRIP6.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:170
+# RUN: yaml2obj --docnum=2 %s -o %t -DNUMRVA=18
+# RUN: llvm-readobj --file-headers %t | FileCheck %s --check-prefix=CHECK3
+# RUN: obj2yaml %t | FileCheck %s --check-prefix=ROUNDTRIP3
----------------
Similar to my comments above, use CHECK18 (and ROUNDTRIP18).


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

https://reviews.llvm.org/D108825