[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:3
+# RUN: llvm-readobj --auxiliary-header %p/Inputs/xlc64-exec 2>&1 | \
+# RUN:   FileCheck -DFILE=xlc64-exec --check-prefixes=XLC64EXEC,WARN64 %s
+
----------------



================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:15
+# RUN: llvm-readobj --auxiliary-header %p/Inputs/xlc32-obj-malform.o 2>&1 | \
+# RUN:   FileCheck  -DFILE=xlc32-obj-malform.o --check-prefixes=XLC32OBJ-PART,WARN-PART %s
+
----------------



================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:17
+
+# XLC32EXEC: File: {{.*}}xlc32-exec
+# XLC32EXEC-NEXT: Format: aixcoff-rs6000
----------------



================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:54
+
+
+# XLC64EXEC: File: {{.*}}xlc64-exec
----------------
Delete extra blank line.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:91
+# XLC64EXEC-NEXT:   Additional flags 64-bit XCOFF: 0x0
+# WARN64:           {{.*}}llvm-readobj{{(\.exe)?}}: 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)
----------------
There's no need to show the tool name is printed here.

Why do you have a warning in a "normal" case?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:122
+# XLC32OBJ-PART-NEXT:   .text section start address: 0x0
+# WARN-PART:   {{.*}}llvm-readobj{{(\.exe)?}}: warning: '{{.*}}[[FILE]]': only partial field for .data section start address at offset (24)
+# XLC32OBJ-PART-NEXT:   Raw data: (00 00 02)
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:19
 
 #include <stddef.h>
 
----------------
jhenderson wrote:
> This header shouldn't have been added in your previous patch. Please delete it here.
Ping. The header is still there. Please delete it, and please stop marking things as Done when they haven't been addressed.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:607
+static auto PrintAuxMemberHelper =
+    [](PrintStyle Ps, const char *MemberName, auto &Member, auto &AuxHeader,
+       uint16_t AuxSize, uint16_t &PartialFieldOffset,
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Don't abbreviate unnecessarily. It harms readability.
> > 
> > I previously suggested you looked at `std::bind` to allow you to pass in the print* function, rather than having to have the `PrintStyle` enum. Did you look at doing that?
> sorry that std::bind do not work for a template function.  it will compile error
You've not addressed my comment about the parameter name. Also, the function name shouldn't start with a capital letter.

> sorry that std::bind do not work for a template function. it will compile error

Are you sure? (see e.g. https://stackoverflow.com/questions/26981608/how-to-use-template-function-parameter-in-stdbind).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116220