jhenderson added a comment.
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/test/tools/llvm-readobj/XCOFF/auxiliary-header.test:2
+## This file tests the ability of llvm-readobj to display the auxiliary header for 64 bits XCOFF and 32 bits XCOFF object file.
+# RUN: llvm-readobj --auxiliary-header %p/Inputs/xcoff-64-xlc-exec 2>&1 | \
+# RUN: FileCheck -DFILE=xcoff-64-xlc-exec --check-prefixes=XLC64EXEC,WARN64 %s
----------------
I previously suggested test input files should be renamed too, i.e. `64-xlc-exec` since it's already in an XCOFF subdirectory.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:19
#include <stddef.h>
----------------
This header shouldn't have been added in your previous patch. Please delete it here.
================
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,
----------------
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?
================
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,
----------------
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?
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:610
+ const char *&PartialFieldName, ScopedPrinter &W) {
+ ptrdiff_t Offset = (const char *)(&Member) - (const char *)(AuxHeader);
+ if (Offset + sizeof(Member) <= AuxSize)
----------------
Don't use C-style casts. Use `static_cast` and `reinterpret_cast`.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:621
+template <class T>
+void CheckAndPrintAuxHeaderParseError(const char *PartialFieldName,
+ uint16_t PartialFieldOffset,
----------------
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:625
+ XCOFFDumper *Dumper) {
+
+ if (PartialFieldOffset < AuxSize) {
----------------
Don't start functions with a blank line.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:628
+ std::string ErrInfo;
+ llvm::raw_string_ostream StringOS(ErrInfo);
+ StringOS << "only partial field for " << PartialFieldName << " at offset ("
----------------
No need to provide `llvm::` namespace qualifier.
However, I previously pointed out you don't want a string stream here at all. From my comment in D82549 (with a couple of minor fixes):
```
std::string ErrInfo = (Twine("only partial field for ") + PartialFieldName + " at offset (" + PartialFieldOffset + ")").str();
```
Alternatively, you could just do:
```
Dumper->reportUniqueWarning(Twine("only partial field for ") + PartialFieldName + " at offset (" + PartialFieldOffset + ")");
```
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:635
+ "Raw data", "",
+ ArrayRef<uint8_t>((const uint8_t *)(&AuxHeader) + PartialFieldOffset,
+ AuxSize - PartialFieldOffset));
----------------
Use reinterpret_cast/static_cast, not C-style casts.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:642
+ "Extra raw data", "",
+ ArrayRef<uint8_t>((const uint8_t *)(&AuxHeader) + sizeof(AuxHeader),
+ AuxSize - sizeof(AuxHeader)));
----------------
C++ style cast needed.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:655
DictScope DS(W, "AuxiliaryHeader");
----------------
You seem to have ignored my comment chain in D82549 about putting this scope into a higher-level function. Yes, this means that there's empty output, if you don't have an auxiliary header, but that's fine, since you explicitly asked for the auxiliary header output. Additionally, that's what is already being done in LLVM-style ELF output.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116220/new/
https://reviews.llvm.org/D116220