[PATCH] D125439: Ensure that the MRI CREATE/CREATETHIN commands overwrite the output file appropriately

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:3
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: touch %t/archive.a
----------------
Let's cd into %t, then we don't need all these relative paths involving %t.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:4
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: touch %t/archive.a
+# RUN: touch %t/1.txt
----------------
I recommend you move this line down to the first test case, as it's part of that specific test's setup code, and not a generic thing used by all the test cases.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:9
+## Show that an existing file that is not an archive is overwritten by CREATE.
+# RUN: echo "CREATE %t/archive.a" > %t/test.mri
+# RUN: echo "ADDMOD %t/1.txt" >> %t/test.mri
----------------
If you cd into %t, you can use split-file instead of echo, since you don't need to dynamically generate the script contents. You can also avoid the need for touch to create the initial files, if you want, though that's less of an issue.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:15
+# RUN:   FileCheck %s --check-prefix=CREATE
+CREATE: 1.txt
+
----------------
Check patterns are usually prefixed with a comment marker too, and are often then separated with a blank line to help them stand out. Aplpies throughout.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:26
+# RUN:   FileCheck %s --check-prefix=CREATETHIN
+CREATETHIN: 1.txt
+
----------------
No need for a separate check pattern here. You can reuse the existing one.

Looking through all the test cases, I think you could get away with sharing the following block for all non-error cases:

```
# ARCH: !<arch>
# THIN:  !<thin>
# ONE:  1.txt
# TWO:  2.txt
```

and then use the appropriate check-prefixes incantation for the specific test case. Also, could you make the 1.txt and 2.txt lines -NEXT patterns?


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:28
+
+## Show that an existing full archive is overwritten by CREATE.
+# RUN: rm -f %t/archive.a
----------------
If you were to move this test case before the CREATETHIN case above, you would already have a regular archive so wouldn't need to delete it and recreate it. Similar comments probably apply throughout the test file.

Additionally, here and throughout, the canonical term is "regular archive" not "full archive". Please remove all references to "full" from comments and check-directives.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:36
+# RUN: llvm-ar tv %t/archive.a
+# RUN: FileCheck -input-file=%t/archive.a %s --check-prefix=FULL_CREATE --implicit-check-not=1.txt
+FULL_CREATE: !<arch>
----------------
Be consistent with double versus single dashes. Also, may not be relevant, but I'd prefer dashes to underscores in prefix names, as they're a) easier to type, and b) consistent with most other tests.

Applies throughout.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:79
+
+## Show that the output is not overwritten without a SAVE.
+# RUN: echo "CREATE %t/archive.a" > %t/test.mri
----------------
It seems like it would just be easier to show that archive.a still has it's old contents than make it read-only?


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:84-88
+## Show that a read-only file is not overwritten.
+# RUN: echo "SAVE" >> %t/test.mri
+# RUN: not llvm-ar -M < %t/test.mri 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=ERROR -DFILE=%t/archive.a
+# ERROR: error: [[FILE]]: permission denied
----------------
This seems to be somewhat tangential to what the test is doing: this is more about handling errors on SAVE, one of which might be a read-only file, but presumably there are other possible failure cases, e.g. disk space or bad paths that have nothing to do with the overwriting behaviour, but all go through the same code path on `SAVE`?


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:100
+SECOND: 2.txt
+
----------------
Nit: looks like you've got a double blank line at EOF?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1137
   if (Saved)
-    performOperation(ReplaceOrInsert, &NewMembers);
+    performOperation(ReplaceOrInsert, nullptr, nullptr, &NewMembers);
   exit(0);
----------------
I'd add comment labels for the two nullptr args, to make it clear what they are for (fake examples suggested in the edit, to show formatting - names should match function parameter names).


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

https://reviews.llvm.org/D125439