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

Thu May 12 07:29:06 PDT 2022

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:
> 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.

================
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
----------------
jhenderson wrote:
> 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.
Thanks. Done.

================
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:
> 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.

================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:15
+# RUN:   FileCheck %s --check-prefix=CREATE
+CREATE: 1.txt
+
----------------
jhenderson wrote:
> 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.
Done. Thanks!

================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:26
+# RUN:   FileCheck %s --check-prefix=CREATETHIN
+CREATETHIN: 1.txt
+
----------------
jhenderson wrote:
> 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?
Thanks this makes the test more readable.

> Also, could you make the 1.txt and 2.txt lines -NEXT patterns?

Sadly not, e.g:

1: !<thin>
2: // 72 `
3: C:/u/br2/test/tools/llvm-ar/Output/mri-create-overwrite.test.tmp/1.txt/

================
Comment at: llvm/test/tools/llvm-ar/mri-create-overwrite.test:26
+# RUN:   FileCheck %s --check-prefix=CREATETHIN
+CREATETHIN: 1.txt
+
----------------
bd1976llvm wrote:
> jhenderson wrote:
> > 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?
> Thanks this makes the test more readable.
> 
> > Also, could you make the 1.txt and 2.txt lines -NEXT patterns?
> 
> Sadly not, e.g:
> 
> 1: !<thin>
> 2: // 72 `
> 3: C:/u/br2/test/tools/llvm-ar/Output/mri-create-overwrite.test.tmp/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
----------------
jhenderson wrote:
> 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.
I have changed terminology from full -> regular. Thanks.

Unless you feel strongly, I think that it is nice to keep the teste-cases self-contained rather than relying on state from the last test case. It makes the test easier to read.

================
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>
----------------
jhenderson wrote:
> 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.
Thanks. Done.

================
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
----------------
jhenderson wrote:
> It seems like it would just be easier to show that archive.a still has it's old contents than make it read-only?
Done. Thanks.

================
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
----------------
jhenderson wrote:
> 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`?
Agreed. I have remove the test-case. Thanks.

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

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

https://reviews.llvm.org/D125439