# [CommandLine] Remove OptionCategory and SubCommand caches from the Option class.ClosedPublicActions

Authored by hintonda on May 18 2019, 9:59 PM.
Tags
• Restricted Project
• Restricted Project
Subscribers

# Details

Summary

This change processes OptionCategorys and SubCommands as they
are seen instead of caching them in the Option class and processing
them later. Doing so simplifies the work needed to be done by the Global
parser and significantly reduces the size of the Option class to a mere 64
bytes.

Removing the OptionCategory cache saved 24 bytes, and removing
the SubCommand cache saved an additional 48 bytes, for a total of a
72 byte reduction.

# Diff Detail

Repository
rG LLVM Github Monorepo
Build Status
 Buildable 32121 Build 32120: arc lint + arc unit

### Event Timeline

hintonda created this revision.May 18 2019, 9:59 PM
Herald added a project: Restricted Project. May 18 2019, 9:59 PM
Herald added a subscriber: hiraditya.

The Option class started at 184 bytes, this change, along with D62091, gets it down to 88 types, and reduces bin/opt by over 90k.

hintonda retitled this revision from [CommandLine} Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove SubCommands SmallPtrSet from the Option class..May 18 2019, 10:10 PM
hintonda updated this revision to Diff 200183.May 19 2019, 10:49 AM
• Add comments, fix auto usage, and minor refactoring.
hintonda updated this revision to Diff 200185.May 19 2019, 1:23 PM
• Remove Categories cache as well. This change reduces the size of the Option class down to 64 bytes.
hintonda retitled this revision from [CommandLine] Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..May 19 2019, 1:28 PM
hintonda edited the summary of this revision. (Show Details)
hintonda edited the summary of this revision. (Show Details)
beanz added a subscriber: zturner.

Adding @zturner in case he has any thoughts.

tstellar added a subscriber: serge-sans-paille.
llvm/include/llvm/Support/CommandLine.h
309

Why do you need to make that getter public?

hintonda marked an inline comment as done.May 23 2019, 11:04 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
309

I think at some point I needed access, but as of now, FullyInitialized is only used within Option, so it can go away.

Thanks for catching that.

hintonda updated this revision to Diff 201049.May 23 2019, 1:03 PM
• Removed unneeded public method from Option.
This revision is now accepted and ready to land.May 31 2019, 9:55 AM
hintonda updated this revision to Diff 202622.EditedJun 2 2019, 1:37 PM

Since a SubCommand's OptionsMap key is a string, and multiple
SubCommands can have unrelated options of the same name,
getSubCommands needs to also make sure the found Option matches the
this pointer.

Also, hid the addArgument() method in Alias to prevent users from
trying to add it twice, once when specifying a SubCommand, and again
in the done() method.

Added tests to cover these two cases -- first discovered in llvm-xray,
which declares duplicate Options and Aliases in multiple SubCommands.

hintonda marked an inline comment as done.Jun 3 2019, 8:17 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1796

Will fix this and related methods by making them virtual and adding asserts saying the operation isn't allowed/doesn't make sense in a separate patch.

MaskRay added inline comments.Jun 3 2019, 11:05 PM
llvm/include/llvm/Support/CommandLine.h
290
1. Does it have to be so large?
hintonda marked an inline comment as done.Jun 5 2019, 6:55 PM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
290

No, it should probably be 1 or 2, and getSubCommands should probably be 1.

Thanks for pointing this out.

hintonda updated this revision to Diff 204984.Jun 16 2019, 6:42 PM
• Reduce compile-time container sizes.
hintonda updated this revision to Diff 205092.Jun 17 2019, 8:55 AM
• Make Option::addArgument virtual.

Even though this patch has been accepted, I've made 2 additional changes, and would
like to have them reviewed as well.

The first just reduces the compile-time default container sizes for the get* methods,
which shouldn't be too controversial.

However, the second is a bit more involved, and can be viewed in the following two
diffs -- the second of which just makes addArgument virtual.

```https://reviews.llvm.org/D62105?vs=201049&id=202622&whitespace=ignore-most#toc
https://reviews.llvm.org/D62105?vs=204984&id=205092&whitespace=ignore-most#toc```

It fixes bugs in the following two use cases (please see the two additional uinittests that
cover these):

• When a users incorrectly passes a cl::sub argument to an Alias ctor:

Since Alias's use the SubCommands of the Option they alias, this argument should be ignored, or assert.
• When using SubCommands with duplicate Alias's, within the scope of a SubCommand:

In order to distinguish which SubCommand the matching Alias belongs, it is necessary to compare the found option to this.
llvm/include/llvm/Support/CommandLine.h
1796

Why do you want to do that in a separate patch?

hintonda marked an inline comment as done.Jun 17 2019, 10:06 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1796

Sorry, that comment is OBE. I made it virtual instead.

My original reasoning was to make each patch smaller, but found breaking it up further un-workable.

This revision was automatically updated to reflect the committed changes.
hintonda reopened this revision.Jul 9 2019, 7:26 AM

Reopening to track fix for buildbot failure -- need to make GeneralCategory a ManagedStatic.

This revision is now accepted and ready to land.Jul 9 2019, 7:26 AM

Patch is ready, so I'll post as soon as I complete rebase/build/test.

The fix is just making GeneralCategory a ManagedStatic, so without objection, I'll land it tomorrow and watch the bots.

hintonda updated this revision to Diff 208736.Jul 9 2019, 10:31 AM

Make GeneralCategory a ManagedStatic.

Herald added a project: Restricted Project. Jul 9 2019, 10:31 AM
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
This revision was automatically updated to reflect the committed changes.