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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 13 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:37
+# RUN: llvm-ar -q -c -X 64 archive.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1 | \
+# RUN:  FileCheck %s  --check-prefixes=WARN-32
+# RUN: llvm-ar -t -Xany archive.a | \
----------------
jhenderson wrote:
> You've still got a double space between `%s` and the `--check-prefixes` option here. Please remove it from here and all other cases.
> 
> In general, when I make a comment, please make sure you've addressed all occurrences of the issue I raised, rather than just the one, as it will speed up the review and save us both time.
thanks , I will do it.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:179
+
+#OBJ32:          xcoff32.o 
+#OBJ32-NEXT:     elf32.o
----------------
jhenderson wrote:
> Please remember to include a space between your `#` and the check directive.
thanks


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:200
+
+## Test -X option for object file bit code,mach-O,coff,wasm
+
----------------
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?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:670-671
+    return true;
+  LLVMContext Context;
+  Context.setOpaquePointers(true);
+  Expected<std::unique_ptr<Binary>> BinOrErr = getAsBinary(Member, &Context);
----------------
jhenderson wrote:
> I'm not sure I follow the need for this bit of code? Has something else been rebased that requires it, and if so, could you explain why it's correct to use it the way you've done (i.e. with the `setOpaquePointers(true)`, please.
we add bitcode object file test in test case ,it need a  LLVMContext  to createBinary for bit code.
https://github.com/llvm/llvm-project/blob/ca2933f3f88a09eb485146eef776b7345e3d87d3/llvm/lib/Object/SymbolicFile.cpp#L48


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864