[PATCH] D73820: [llvm-strip][WebAssembly] Support strip flags

Tue Jul 13 08:30:26 PDT 2021

sbc100 added a comment.

lgtm with comments addressed

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-debug.test:1
+## Test that debug sections are stripped with --strip-debug
+# RUN: yaml2obj %s -o %t
----------------
jhenderson wrote:
> dschuff wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > dschuff wrote:
> > > > > sbc100 wrote:
> > > > > > Can you do a followup that renames all the `.test` files in this directory to `.yaml` .. its more descriptive and enables syntax highlighting .
> > > > > Maybe? ELF and COFF use .test instead of .yaml
> > > > I have no problem with ELF and COFF tests being renamed too, but beware that by default, lit doesn't recognise files ending with `.yaml` extensions as tests.
> > > > I have no problem with ELF and COFF tests being renamed too, but beware that by default, lit doesn't recognise files ending with `.yaml` extensions as tests.
> > > 
> > > I think that this comment doesn't apply any more - try running lit in the directory, with the tests renamed, and see what happens (don't explicitly specify the test themselves, as lit will then run them regardless of their extension).
> > I checked, and the same number of tests are discovered when the extension is .yaml or .test. So if you want I can rename all of the .test tests to .yaml in a followup change.
> I don't care either way - if there's a desire to do it, I don't oppose doing so.
I agree that landing this with the same convention as the surrounding files makes sense.

Whether you want to do the mass renaming before or after this lands I don't this it matters.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820