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

Tue Oct 12 15:23:40 PDT 2021

DiggerLin added inline comments.

================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:548
+      AuxiSize)                                                                \
+  W.print##H(S, AuxHeader->T)
+
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > 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.
> Testing should be in the same patch as the code it is testing. Please don't split them into separate patches. If the functionality can be added standalone, and there's a sensible behaviour (even if that behaviour is "do nothing") without it, then that functionality and testing should be in a separate patch.
added a new test for this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549