[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 23 07:34:08 PST 2021
DiggerLin marked 12 inline comments as done.
DiggerLin added inline comments.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:735
+
+ if (PartialFieldOffset < AuxSize) {
+ std::string ErrInfo;
----------------
jhenderson wrote:
> This block looks very similar to the equivalent 32-bit version. Could you move it into a function?
create a help function.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:605
+
+#define PrintAuxMember32(H, S, T) \
+ if (offsetof(XCOFFAuxiliaryHeader32, T) + \
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > DiggerLin wrote:
> > > > jhenderson wrote:
> > > > > Could this not just be a lambda? That would play friendlier with IDEs if nothing else.
> > > > the T is a member of class XCOFFDumper.cpp , I do not think C++11 support a template lambda.
> > > Not sure I quite follow, but note that LLVM supports C++14 and `auto` can be used to much the same effect, if I'm not mistaken. I believe something like the following might work (note: I haven't tested, and am not particularly familiar with auto lambdas).
> > > ```
> > > enum PrintStyle { Hex, Number };
> > > auto PrintAuxMember32 = [&](PrintStyle P, StringRef S, auto *T) {
> > > ptrdiff_t Offset = T - AuxHeader;
> > > if (Offset + sizeof(*T) <= AuxSize) {
> > > switch(P) {
> > > case Hex:
> > > W.printHex(S, *T);
> > > break;
> > > case Number:
> > > W.printNumber(S, *T);
> > > break;
> > > }
> > > } else if (Offset < AuxSize) {
> > > PartialFieldOffset = Offset;
> > > PartialFieldName = S;
> > > }
> > > };
> > >
> > > PrintAuxMember32(Hex, "Magic", &AuxHeader->AuxMagic);
> > > PrintAuxMember32(Hex, "Version", &AuxHeader->Version);
> > > // etc...
> > > ```
> > > You might be able to avoid the enum using some sort of std::bind call or similar, which would be nicer, but not essential.
> > >
> > > Even without that, I think you should factor out the repeated `offsetof` calls into a variable, and use more meaningful variable names rather than `H`, `S` and `T`, just like you would for any function.
> > there is an article about the auto for c++14. https://solarianprogrammer.com/2014/08/21/cpp-14-auto-tutorial/
> >
> > G++ 4.9.1 also supports unconstrained generic functions, basically you can use auto in a function parameter list, so the above code can be further simplified as :
> >
> > 1 auto& add_one(auto& v) {
> > 2 for(auto& it : v) {
> > 3 it += 1;
> > 4 }
> > 5 return v;
> > 6 }
> > 7
> > 8 void multiply_by_two(auto& v) {
> > 9 for(auto& it : v) {
> > 10 it *= 2;
> > 11 }
> > 12 }
> > unfortunately, this didn’t entered in the final C++14 standard. It is expected to be added later to the standard as a technical report.
> >
> > I test the code in the clang,
> > the 'auto' not allowed in function prototype
> >
> > @jhenderson
> You've missed where I've said specifically about lambdas. Please see https://en.cppreference.com/w/cpp/language/lambda. Specifically, look for "generic lambdas". Generic lambdas can take auto as type parameters, and are essentially equivalent to a templated function with the type specified as a template parameter (templated lambdas aren't in the C++14 standard, but generic lambdas mean they often aren't needed). I am //**not**// talking about auto in a function parameter list.
thanks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82549/new/
https://reviews.llvm.org/D82549
More information about the llvm-commits
mailing list