This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Add -p as alias to -pretty-print
ClosedPublic

Authored by Quolyk on Jan 10 2019, 6:04 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Quolyk created this revision.Jan 10 2019, 6:04 AM
Quolyk edited the summary of this revision. (Show Details)Jan 10 2019, 6:12 AM
Quolyk added a reviewer: jhenderson.

Thanks for working on this!

test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #181039)

Nit, here and in the line above we're being inconsistent with the number of '-' characters used to prefix options (single dash for llvm-symbolizer, double for FileCheck). It would be nice if they were consistent. I have a marginal preference for double dash, but I don't mind if you want to go the other way.

tools/llvm-symbolizer/llvm-symbolizer.cpp
86 ↗(On Diff #181039)

It looks like this line needs clang-formatting.

Aliases aren't in the help text by default, but you can make them be by adding the cl::NotHidden attribute. I think you should do that.

Quolyk updated this revision to Diff 181050.Jan 10 2019, 7:00 AM

Add cl::NotHidden option

Quolyk marked 2 inline comments as done.Jan 10 2019, 7:03 AM
Quolyk added inline comments.
test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #181039)

This is an opinionated question and I shall do as it comfortable to others for any historical reason.

Okay, looks good once the issue with the dashes is fixed.

jhenderson added inline comments.Jan 10 2019, 7:08 AM
test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #181039)

I'm not sure I understand what you mean? I think that there was no good reason to use a mixture of single and double dashes before.

You can change the other lines in the test to make them all match.

Quolyk updated this revision to Diff 181058.Jan 10 2019, 7:13 AM

FileCheck --check-prefix --> FileCheck -check-prefix

This revision is now accepted and ready to land.Jan 10 2019, 7:15 AM
Quolyk marked an inline comment as done.Jan 10 2019, 7:19 AM
Quolyk added inline comments.
test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #181039)

I agree, will take in account in future. I skimmed other tests and notices this pattern.

This revision was automatically updated to reflect the committed changes.