[PATCH] D106783: [AIX][XCOFF][llvm-readobj] Replace unwrapOrError with reportUniqueWarning

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml:1
+## Test that we can explicitly specify all the fields.
+# RUN: yaml2obj %s -o %t
----------------
This comment looks like it's been copied from somewhere else and doesn't make sense in this context. Please rewrite it.

The test name suggests this is for generic dumping of 32-bit relocations, but I think your patch is about reporting of warnings, so the test name doesn't make much sense to me.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml:5-8
+# CHECK:      File: {{.*}}relocations32.yaml.tmp
+# CHECK-NEXT: Format: aixcoff-rs6000
+# CHECK-NEXT: Arch: powerpc
+# CHECK-NEXT: AddressSize: 32bit
----------------
If this test is just about checking the warnings, you can omit these 4 lines, as they aren't important.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml:11
+# CHECK-NEXT:   Section (index: 2) .data {
+# CHECK-NEXT:     {{.*}} warning: {{.*}} Invalid symbol index
+# CHECK-NEXT:   }
----------------
What's the full message here that you are matching?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml:17
+FileHeader:
+  MagicNumber:          0x1DF
+Sections:
----------------
We generally remove execessive whitespace, so that values within a block line up vertically, but as far left as possible.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml:20
+  - Name:                    .text
+    Flags:                   [ STYP_TEXT ]
+  - Name:                    .data
----------------
I don't think you need this section?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:151
+      if (Error E = ErrOrSymbolName.takeError())
+        reportUniqueWarning(std::move(E));
 
----------------
jhenderson wrote:
> Same comments as above.
This still seems to be without a test?


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

https://reviews.llvm.org/D106783