This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Allow single letter command flags grouping.
ClosedPublic

Authored by Quolyk on Jan 22 2019, 4:35 AM.

Details

Summary

Currently llvm-symbolizer doesn't allow flags combining. This patch allows such grouping behavior just like addr2line. Motivation: https://bugs.llvm.org/show_bug.cgi?id=40304

Diff Detail

Repository
rL LLVM

Event Timeline

Quolyk created this revision.Jan 22 2019, 4:35 AM

The code change looks fine. Please make sure, if you haven't already, to review all tests using llvm-symbolizer (they exist both in tools/llvm-symbolizer and DebugInfo) that currently use single dash options (for long options) to make sure they aren't negatively impacted. Adding grouping behaviour can cause a change in behaviour for these cases, if you're not careful.

test/tools/llvm-symbolizer/sym.test
25 ↗(On Diff #182887)

Rather than extending an existing test, I'd prefer it if this was done in its own test, as it isn't necessarily clear that this is specifically testing the combining of short options.

Quolyk updated this revision to Diff 182904.Jan 22 2019, 6:24 AM

Move test to separate file.
llvm/test/tools/llvm-symbolizer/ and llvm/test/DebugInfo/llvm-symbolizer* pass successfully.

jhenderson accepted this revision.Jan 22 2019, 7:54 AM

LGTM, with one nit.

test/tools/llvm-symbolizer/flag-grouping.test
7 ↗(On Diff #182904)

Please add a new line at the end of the file.

This revision is now accepted and ready to land.Jan 22 2019, 7:54 AM
jhenderson added inline comments.Jan 22 2019, 7:58 AM
test/tools/llvm-symbolizer/flag-grouping.test
4 ↗(On Diff #182904)

Oops, I didn't look closely enough at the test. The start of the check here is unnecessarily complicated. It can just be:

CHECK: 0x40054d: inctwo etc.

Quolyk updated this revision to Diff 183042.Jan 22 2019, 11:22 PM

Update test.

This revision was automatically updated to reflect the committed changes.