[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 9 inline comments as done.
DiggerLin added inline comments.


================
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)
----------------
jhenderson wrote:
> There's no need to show the tool name is printed here.
> 
> Why do you have a warning in a "normal" case?
please see the comment in 
"We should produce some warning if the auxiliary table size indicates additional content past the last field that the code understands. This is another case where I think we should also output the raw data."
https://reviews.llvm.org/D82549#inline-770356



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:19
 
 #include <stddef.h>
 
----------------
jhenderson wrote:
> 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.
thanks


================
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,
----------------
jhenderson wrote:
> 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).
if you look into the link info,  when using std::bind, it need to explicit instantiate  the template PrintAuxMemberHelper function. 

there is info as 
"Execute_For is a function template, therefore you need to either supply it with type template arguments"

and 

https://stackoverflow.com/questions/14727313/c-how-to-reference-templated-functions-using-stdbind-stdfunction

https://en.cppreference.com/w/cpp/utility/functional/bind

"f	-	**Callable object** (function object, pointer to function, reference to function, pointer to member function, or pointer to data member) that will be bound to some arguments"

if we need to initiate the template , we need to define several helper function witch std::bind, what is the benefit of using std::bind ?

I need to define something as 


```
  using namespace std::placeholders;

  auto PrintAuxMember16 = std::bind(PrintAuxMemberHelper<support::ubig16_t,XCOFFFileHeader32>, _1, _2, _3, AuxHeader,AuxSize,PartialFieldOffset, PartialFieldName, W);

  auto PrintAuxMember32 = std::bind(PrintAuxMemberHelper<support::ubig32_t,XCOFFFileHeader32>, _1, _2, _3, AuxHeader,AuxSize,PartialFieldOffset, PartialFieldName, W);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116220