[PATCH] D129088: [llvm-ar][test] Add testing for bitcode file handling

[Owen Reynolds via Phabricator via llvm-commits]

gbreynoo marked 8 inline comments as done.
gbreynoo added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:38
+
+## Update symtab from bitcode in an existing thin archive.
+# RUN: llvm-ar rS --thin update-thin.a a.bc
----------------
jhenderson wrote:
> These "update" test cases are about creating a symbol table in an existing archive, but the test is called "bitcode-add.ll". Is the test misnamed? These update cases, aren't really "adding" anything.
I agree the test is poorly named, hopefully the more general name is more fitting?


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:60
+## Extract bitcode and ensure it has not been changed.
+# RUN: cd %t/extracted
+# RUN: llvm-ar x %t/new.a a.bc
----------------
jhenderson wrote:
> Again, not a test case to do with "adding" anything.
I forgot to change these uses of %t, I'll fix it.


================
Comment at: llvm/test/tools/llvm-ar/bitcode-add.ll:60-62
+# RUN: cd %t/extracted
+# RUN: llvm-ar x %t/new.a a.bc
+# RUN: diff a.bc %t/a.bc
----------------
gbreynoo wrote:
> jhenderson wrote:
> > Again, not a test case to do with "adding" anything.
> I forgot to change these uses of %t, I'll fix it.
As mentioned above.


================
Comment at: llvm/test/tools/llvm-ar/mri-bitcode.ll:1
+## Show that when bitcode is added to an archive via mri script it is
+## handled correctly. The symbol table is as expected and it can be
----------------
jhenderson wrote:
> This test basically looks identical to the other test, except that MRI scripts are used to drive the program. I feel like it would be better to include the MRI cases alongside the non-MRI cases in the same test file.
Agreed, I've combined them.


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

https://reviews.llvm.org/D129088