[PATCH] D116220: [AIX][XCOFF] address post-commit review comments of patch https://reviews.llvm.org/D82549

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:17
+
+# XLC32EXEC:      File: {{.*}}xlc32-exec
+# XLC32EXEC-NEXT: Format: aixcoff-rs6000
----------------
You might be able to use `[[FILE]]` here too, instead of the wildcard regex.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:90
+# XLC64EXEC-NEXT:   Additional flags 64-bit XCOFF: 0x0
+# WARN64:           warning: '{{.*}}[[FILE]]': there is extra data after the auxiliary header 
+# XLC64EXEC-NEXT:  Extra raw data: (00 00 00 00 00 00 00 00 00 00)
----------------
Please delete the wildcard regex, as per my original suggestion.

Yes, my question isn't whether we should be printing the warning, but rather whether the normal case should have the warning. I don't think it should, and instead the warning case should be tested explicitly in a separate case. This is a common testing pattern used throughout: handle the normal case, and then have additional cases for errors and warnings.

Aside: if the xlc tool is explicitly generating data that llvm-readobj can't understand, then either llvm-readobj is missing some functionality, or there's a bug in xlc. I'm guessing it's llvm-readobj...


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:121
+# XLC32OBJ-PART-NEXT:   .text section start address: 0x0
+# WARN-PART:            warning: '{{.*}}[[FILE]]': only partial field for .data section start address at offset (24)
+# XLC32OBJ-PART-NEXT:   Raw data: (00 00 02)
----------------
Ditto, please remove the wildcard regex.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:744
+template <typename T, typename V>
+static void PrintAuxMemberHelper(PrintStyle Style, const char *MemberName,
+                                 const T &Member, const V *AuxHeader,
----------------
> Also, the function name shouldn't start with a capital letter.

Still not addressed. Please use lowerCameCase for function names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116220