[PATCH] D127864: [llvm-ar] Add object mode option -X for AIX

Tue Jun 28 13:45:09 PDT 2022

DiggerLin added inline comments.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:200
+
+## Test -X option for object file bit code,mach-O,coff,wasm
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I think you're trying to say "Test -X option with other output formats." right? You don't need to list them out specifically (the test case is clear enough).
> > 
> > Is there a reason you've got ELF in the earlier test cases, rather than down here? I think it would make the earlier tests simpler to use just one file format in the earlier test cases (probably XCOFF) and then just this simple test for the other ones (including ELF).
> > 
> > If you go down this route, make sure you don't lose test coverage that shows that llvm-ar warns for every object it doesn't process, not just the first one. This could be achieved by checking the warnings in this test case, I believe.
> > Is there a reason you've got ELF in the earlier test cases, rather than down here? I think it would make the earlier tests simpler to use just one file format in the earlier test cases (probably XCOFF) and then just this simple test for the other ones (including ELF).
> 
> there is no special reason to got the  ELF in the earlier test cases, but it is no harm to ELF in the earlier test case. And if only letting the earlier test cases only test  xcoff32.o and xcoff64.o, I think it is too simple, so I put the earlier  test cases test on the xcoff32.o elf32.o xcoff64.o elf64.o
> 
> 
> ```
> If you go down this route, make sure you don't lose test coverage that shows that llvm-ar warns for every object it doesn't process, not just the first one. This could be achieved by checking the warnings in this test case, I believe
> ```
> when we checking the warning functionality of  "is not valid with the current object file mode" , I think we only need to check once, we do not need to check everywhere ? is it correct?
And if only letting the earlier test cases only test xcoff32.o and xcoff64.o, I think it is too simple to test especially when testing move operation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864