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

In D116220#3222197 <https://reviews.llvm.org/D116220#3222197>, @jhenderson wrote:

> Your patch summary seems incorrect to me, compared to what it should be. Perhaps simply saying "address post-commit review comments" in the summary is fine. This is also not NFC, as some output has changed slightly (see the "extra data" warning for example). There should be testing for the changed output.
>
> Some of the items I've highlighted (e.g. spurious blank lines, C++ style casts, function case naming) are things that I've asked you to fix many times in other reviews. Please could you take more care when making changes, and consider reviewing your own code first for these sort of things, before putting up patches for wider community review, so that we don't have to waste time pointing them out to you again and again.





================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:606
+enum PrintStyle { Hex, Number };
+static auto PrintAuxMemberHelper =
+    [](PrintStyle Ps, const char *MemberName, auto &Member, auto &AuxHeader,
----------------
jhenderson wrote:
> My original suggestion was that this should be a lambda //witihin// the relevant methods, so that appropriate members could be captured. However, you've pulled it out of those functions into a standalone variable. I don't really mind having it separate, but please explain to me why this is still a lambda now and not a template function?
change to template function, 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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116220