App::add_set() can crash your application in some cases when printing the help

[SkyToGround]

In App::add_set(), the const std::set<std::string> &options argument will likely crash your application when used as follows:

void SomeClass::AddOption(CLI::App &Parser) {
  std::set<int> SomeSet = {1, 2};
  Parser.add_set("-a", SomeVar, SomeSet, "Some description.");
}

The reason for this is that add_set() uses a reference to options when creating a lambda. When this lambda is called at a later point, SomeSet has been allocated. Instead, the possible options should be copied.

[henryiii]

The reference is kept so that you can modify the set later on as in #114. You can fix it in this case:

Parser.add_set("-a", SomeVar, std::move(SomeSet), "Some description.");

(You'll see there is a && version that does a copy). Normally, you have to keep a reference to SomeVar anyway, so the same is true for SomeSet.

[SkyToGround]

I think the design that you currently have is somewhat problematic as it is dangerous by default, i.e. you have to use std::move to make calling add_set() safe. To retain the functionality that you want to have and make the function call safer, there are at least two options that I can think of.

1. Store the std::set of options as a member variable in class Option so that you can modify it after calling add_set.
2. Add a version of add_set which takes a pointer (raw or better yet, shared) and modify the existing reference-based version to instead copy the std::set. This way, the user has to more actively select the functionality that you mention in Sets by reference #114. If you use a shared pointer, this alternative should also be "safe".

[henryiii]

I like option 2. Option 1 can't be done using the current design - all options are the same. At some point (see the idea board), options could be implemented as Option, and then option 1 would be possible.

[phlptp]

I have come across the same issue or similar issues. I would agree with @SkyToGround the function as is makes it easy to get into trouble. I think instead of option 2, I would just suggest a member function of the Option to modify the set and always take by const reference. That function would update the callback with a new lambda function just like the add_set function and just keep the same names.

[henryiii]

So, the options are currently:

1. Make a new CLI::Set() that has much better support for all of this - I'm planning to try this for Version 1.8
2. Make the add_set that allows modification take a pointer - this is a bit inconsistent with the rest of CLI11
3. Make the add_set that takes a reference have a different name, like add_modifiable_set - this is tempting to try to add for 1.7 as a temporary solution.

So I think I will either leave it as is till 1.8 or take option 3 then hopefully still fix it properly in 1.8.

[henryiii]

Number 1 would address #12, as well, I believe.