[PATCH] D56839: [llvm-objcopy] [COFF] Implement --strip-debug

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht added a comment.

Accepted assuming the test case passes when renamed to .test :)



================
Comment at: test/tools/llvm-objcopy/COFF/strip-debug.yaml:1
+# RUN: yaml2obj %s > %t.in.o
+#
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > rupprecht wrote:
> > > Does this test run? I thought it had to end in ".test" to run.
> > > https://github.com/llvm/llvm-project/blob/master/llvm/test/lit.cfg.py#L25
> > Oh crap, you're right. I'm used to working on tests in e.g. lld, where `.yaml` already is added to that list of suffixes, and where the convention is that if the file actually contains yaml input data, it's named `.yaml`, otherwise `.test`.
> > 
> > I've already committed one test with a `.yaml` suffix though. Do you want me to add a `lit.local.cfg` that adds `yaml` to the list (kind of like https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/lit.local.cfg), or should I rename the existing test to `.test`? (I didn't notice since I've been running the tests with `llvm-lit path/to/llvm-objcopy/COFF/*`.)
> I presume renaming to `.test` is easiest, and matches the ELF backend's tests, so I'll rename the existing test in one NFC commit and rename the newly added tests before I commit them as well.
SGTM -- I think just renaming these to `.test` is better.

I wouldn't strongly object to allowing `.yaml` as a test case extension if others are used to it, but it seems a little weird to me...


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

https://reviews.llvm.org/D56839