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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 4 inline comments as done.
DiggerLin added inline comments.


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


================
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)
----------------
jhenderson wrote:
> 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...
> 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...

  The llvm-readobj is implement is based on the https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.files/XCOFF.htm
 I think the extra raw data is align padding data.


================
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,
----------------
jhenderson wrote:
> > Also, the function name shouldn't start with a capital letter.
> 
> Still not addressed. Please use lowerCameCase for function names.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116220