This is an archive of the discontinued LLVM Phabricator instance.

[llvm-strip] Expose --strip-unneeded option
ClosedPublic

Authored by paulsemel on Jun 6 2018, 3:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Jun 6 2018, 3:43 AM
jhenderson accepted this revision.Jun 6 2018, 4:31 AM

LGTM, with the suggested changes.

test/tools/llvm-objcopy/strip-unneeded.test
7

Please put a new line in before this line. It'll help make the llvm-strip part clearer.

8–9

You don't need to copy the input file here, since we don't use it again at this point.

tools/llvm-objcopy/llvm-objcopy.cpp
586

This comment looks a little out of place now with the changes you and @alexshap are making in this area. It probably needs updating or (re)moving.

This revision is now accepted and ready to land.Jun 6 2018, 4:31 AM
paulsemel updated this revision to Diff 150114.Jun 6 2018, 5:22 AM
paulsemel marked 2 inline comments as done.

Added Jame's suggestions

tools/llvm-objcopy/llvm-objcopy.cpp
586

I think you're right. Maybe we should strip it as it is not really useful ? What do you think @alexshap ?

Hey I'm away but I got a bug report requesting -x as an alias in
llvm-strip. I haven't looked at the code for this patch so it might already
be solved here but I'd appreciate it being added. No worries either way.
I'll just add it when aight get back of it isn't there.

Best,
Jake

Hey I'm away but I got a bug report requesting -x as an alias in
llvm-strip. I haven't looked at the code for this patch so it might already
be solved here but I'd appreciate it being added. No worries either way.
I'll just add it when aight get back of it isn't there.

Best,
Jake

Hi Jake,

Are you talking about this revision : https://reviews.llvm.org/D47750 ?
@alexshap will land it soon I think. Btw @alexshap, I'll wait for you to land it before landing this one :)

Ah yes. Thanks!

This revision was automatically updated to reflect the committed changes.