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

[Digger Lin via Phabricator via llvm-commits]

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