-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
options: Allow OptionsImpl to be constructed from a vector #9147
Conversation
This gives better type safety, and is no less efficient since the overload of TCLAP::CmdLine::parse that takes in argc and argv performs the same copy into a std::vector internally. Adding this constructor just gives more flexibility to creators of OptionsImpl instances. Signed-off-by: Alex Konradi <akonradi@google.com>
/azp run envoy-linux |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks. I re-started the tests since the tsan failure is a known flake. Otherwise, looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again. One small thing and we’re good to go.
camelCase, not PascalCase Signed-off-by: Alex Konradi <akonradi@google.com>
Don't rely on the implementation of the argv/argn constructor on top of the constructor that takes a vector. Have explicit tests of both, and validate the usage of argn to limit the number of elements of argv that are parsed. Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This gives better type safety, and is no less efficient since the
overload of TCLAP::CmdLine::parse that takes in argc and argv performs
the same copy into a std::vector internally. Adding this constructor
gives more flexibility to creators of OptionsImpl instances.
Risk Level: low
Testing: ran unit tests
Docs Changes: n/a
Release Notes: n/a