[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

Wed May 19 00:44:31 PDT 2021

jhenderson added a comment.

In D95505#2767644 <https://reviews.llvm.org/D95505#2767644>, @Esme wrote:

> Addressed comments.

Hi @Esme. Have you attempted to address all my comments? It doesn't look like you have. If you update a patch and haven't finished addressing all comments, please make it clear from the comment what you still plan to do.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml:23
+    Flags:           [ STYP_DEBUG, STYP_DATA ]
+    SectionData:     01110103
+  - Flags:           [ STYP_DWARF, STYP_EXCEPT,  STYP_INFO, STYP_TDATA ]
----------------
Be consistent: above you've quoted the section data, and here you haven't. Pick one style (I'd recommend quoting).

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml:24-25
+    SectionData:     01110103
+  - Flags:           [ STYP_DWARF, STYP_EXCEPT,  STYP_INFO, STYP_TDATA ]
+  - Flags:           [ STYP_TBSS, STYP_LOADER, STYP_TYPCHK, STYP_OVRFLO ]
+Symbols:
----------------
Any particular reason these aren't all one section?

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/full-contents.yaml:67
+# CHECK-NEXT:     Name: .text
+# CHECK-NEXT:   
+# CHECK-NEXT:     PhysicalAddress: 0x0
----------------
Why are there blank lines here and below? Does this test actually pass? I was under the impression that `CHECK-NEXT:` without any contents was an error.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505