This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFC] Refactor printing of short options in help
ClosedPublic

Authored by DavidSpickett on Apr 11 2022, 6:05 AM.

Details

Summary

Instead of building a set twice for optional and required,
build a set for each while walking the options once.

Then take advantage of set being sorted meaning we don't
have to enforce the upper/lower order ourselves.

Just cleaned up the formatting on the later loops.
Combined the if conditions and used a single line if.

Depends on D123501

Diff Detail

Event Timeline

DavidSpickett created this revision.Apr 11 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 6:05 AM
DavidSpickett requested review of this revision.Apr 11 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 6:05 AM
JDevlieghere added inline comments.Apr 11 2022, 9:49 PM
lldb/source/Interpreter/Options.cpp
457

I'm surprised you change this from !empty() to size(). I personally think the former is more readable. Not an issue in practice, but I do believe the STL guarantees ::empty to be O(1) while technically ::size could be O(n), though I think in practice they're both O(1).

!empty instead of size

DavidSpickett marked an inline comment as done.Apr 12 2022, 3:40 AM
DavidSpickett added inline comments.
lldb/source/Interpreter/Options.cpp
457

Fair point. I'll keep to the original style.

DavidSpickett marked an inline comment as done.Apr 12 2022, 3:40 AM
jingham accepted this revision.Apr 29 2022, 10:26 AM

Looks like it will do the job, and this version is definitely easier to read.

Were there any tests for the help output? If not, maybe we should add one.

Other than that, LGTM.

This revision is now accepted and ready to land.Apr 29 2022, 10:26 AM

Were there any tests for the help output? If not, maybe we should add one.

For these bits no, in fact I broke them without realising at first. Hence the tests in the other review as you saw, they'll be landed before this.

Thanks for the reviews!