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

[ben via Phabricator via llvm-commits]

bd1976llvm 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
----------------
jhenderson wrote:
> 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.
Now cd-ing.


================
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
----------------
jhenderson wrote:
> 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.
That worked. Also, there is quite a lot of duplication in the scripts so removing that outweighs keeping the script next to the test-cases.


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

https://reviews.llvm.org/D125439