[PATCH] D116220: [AIX][XCOFF] address post-commit review comments of patch https://reviews.llvm.org/D82549
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 9 07:09:30 PST 2022
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
More information about the llvm-commits
mailing list