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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:486
 
+#define PrintAuxMember(H, S, T, X)                                             \
+  W.print##H(S, T);                                                            \
----------------
hubert.reinterpretcast wrote:
> This macro does not operate within the confines of what a function can do with respect to its caller (it can cause the caller to return early). I do not believe that using a function-like naming style is appropriate. I also do not believe that using such a macro for control flow is desirable.
> 
> You can encode a table (yes, a macro is okay for that) with much the same information:
> (format, description, pointer-to-member, offset in the table past-the-end of the member)
> 
> and use that table in the place where this macro is being invoked.
for the function printNumber is a overload function.  using  a macro, the complie will determine which version of  printNumber  will be used when compile. if using a table, I think  how to make the code call the correct overload version of printNumber based in the parameter type when running may be complicated.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:491
+  W.print##H(S, T);                                                            \
+  if ((X = X - sizeof(T)) == 0)                                                \
+  return
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > This strikes me as extremely hazardous. What if we get a length value that is reflective of a partial field?
> > thanks
> We still have to build with C++14 compilers for the time being. Assigning a large 64-bit value to a 32-bit signed type is verboten. In any case, checking the table size against the last field of the table I described above would avoid this issue.
thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549