[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:6-9
+# ARCH: !<arch>
+# THIN: !<thin>
+# ONE:  1.txt
+# TWO:  2.txt
----------------
CHECK lines are usually after test cases.


================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:3
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: touch %t/archive.a
----------------
bd1976llvm wrote:
> jhenderson wrote:
> > Let's cd into %t, then we don't need all these relative paths involving %t.
> I'm not sure that this is the best approach because then you can't just copy/paste run lines from the lit output. You basically have to "cd" too. I'd rather leave this unless you feel strongly.
That's already the case though: you have to copy and paste the rm/mkdir line (as well as any other setup code), or you're not doing the same thing as the test did.


================
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
----------------
bd1976llvm wrote:
> jhenderson wrote:
> > 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.
> Unless you feel strongly I would rather stick with echo. It's nice to keep all the pieces for a test-case together in the file.
As far as I'm aware, there's nothing stopping you doing that, unless llvm-ar doesn't support comment markers or blank lines in MRI scripts? Assuming it does, I think you can do:

```
# RUN: split-file %s
...
## Test case 1
# RUN: test case setup
# RUN: llvm-ar -M 1.mri
...
#--- 1.mri
<mri contents>

## Test case 2
...
#--- 2.mri
```
and so on.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1055-1056
 
-  performOperation(Operation, nullptr, nullptr, NewMembers);
+  performOperation(Operation, /*OldArchive*/ nullptr, /*OldArchiveBuf*/ nullptr,
+                   NewMembers);
   return 0;
----------------
Don't modify lines unrelated to your change.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1137
   if (Saved)
-    performOperation(ReplaceOrInsert, &NewMembers);
+    performOperation(ReplaceOrInsert, nullptr, nullptr, &NewMembers);
   exit(0);
----------------
jhenderson wrote:
> 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).
I was very deliberate with the `=` after the parameter name (but typo'ed it in the second case): clang-format recognises that specific pattern and doesn't insert a space between the comment and the name, which I find helps with readability.


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

https://reviews.llvm.org/D125439