Page MenuHomePhabricator

[llvm-objdump] add support for '--syms' as an alias of -t (PR39406)
ClosedPublic

Authored by Higuoxing on Oct 28 2018, 8:12 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Higuoxing created this revision.Oct 28 2018, 8:12 PM
Higuoxing retitled this revision from add support for '--syms' as an alias of -t (PR39406) to [llvm-objdump] add support for '--syms' as an alias of -t (PR39406).Oct 28 2018, 8:54 PM
jhenderson added inline comments.Oct 29 2018, 4:01 AM
test/tools/llvm-objdump/common-symbol-elf.test
1 ↗(On Diff #171448)

I don't think this is the right place to add this sort of test. The test is specifically about common symbols, not about generic behaviour of the -t switch and how the command-line is handled.

Perhaps a better thing to do would be something similar to what WebAssembly/symbol-table.test does, but add a new test for ELF, e.g. elf-symbol-table.test, which takes both arguments.

Higuoxing updated this revision to Diff 171627.Oct 29 2018, 7:26 PM
Higuoxing added a reviewer: eush.

Update test cases

eush requested changes to this revision.Oct 29 2018, 11:33 PM

I would suggest renaming the test to symbol-table-elf.test for consistency with other *-elf tests in this directory.

test/tools/llvm-objdump/elf-symbol-table.test
5 ↗(On Diff #171627)

I think this check should be removed because file format line is not the focus of this test.

This revision now requires changes to proceed.Oct 29 2018, 11:33 PM

I would suggest renaming the test to symbol-table-elf.test for consistency with other *-elf tests in this directory.

I agree with this, but I think a separate change might be worthwhile to rename all of the *-elf tests to elf-*, as that better matches the majority of the coff-* and macho-* tests.

Otherwise, looks good, with the suggestions @eush has made.

Higuoxing updated this revision to Diff 171670.Oct 30 2018, 4:19 AM

Thanks a lot!

eush accepted this revision.Oct 30 2018, 7:45 AM
This revision is now accepted and ready to land.Oct 30 2018, 7:45 AM

LG and improves test coverage as well.

Please mention if you need someone to land the patch on your behalf after review, makes it easier, especially for smaller patches.

Hi James, @jhenderson , could you please help me commit this patch? Thanks a lot :)

Thank you everyone :)

Hi James, @jhenderson , could you please help me commit this patch? Thanks a lot :)

Thank you everyone :)

If you want I can test it and land it if there are no issues with the lit test. Will be a moment.

Hi James, @jhenderson , could you please help me commit this patch? Thanks a lot :)

Thank you everyone :)

If you want I can test it and land it if there are no issues with the lit test. Will be a moment.

Thanks a lot, please help me :-)

This revision was automatically updated to reflect the committed changes.