[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:605
+
+#define PrintAuxMember32(H, S, T)                                              \
+  if (offsetof(XCOFFAuxiliaryHeader32, T) +                                    \
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549