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

Tue Jul 14 09:59:53 PDT 2020

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.

================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:548
+      AuxiSize)                                                                \
+  W.print##H(S, AuxHeader->T)
+
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > DiggerLin wrote:
> > > hubert.reinterpretcast wrote:
> > > > Please indent the body of the `if`. Also, if `AuxiSize` is greater than the offset of the field but less than the offset past-the-end of the field, then we have a partial field and should probably produce some warning about it (in case the producer of the input file has behaviour we did not expect). I would also suggest to output the raw data in that case.
> > > I agree your suggestion that "we have a partial field and should probably produce some warning about it " . If I implement the suggestion in the the patch, I also need to add test case to test  it.  It maybe cause the patch too big, I think it maybe better to put the suggestion in other patch.
> > I would suggest implementing it in this patch and to post another patch (while this review is still up) with the specific testing for the error cases of having partial fields or trailing unrecognized fields.
> without the test case , I do not think we can 100% guarantee the code is correct. 
> if  when we write a test case for it. we find an  error on the code, we would put the fix and specific test case in the some patch?
Well, if the original patch has not already landed, I do not see why we wouldn't apply the fix in the original patch. As for the testing, I think it should be fine to have it in a separate patch that is intended to land at around the same time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549