Skip to content
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.

Sign up for GitHub

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

Jump to bottom

app-template: add ability to specify options_description to be used for --help #2421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

app-template: add ability to specify options_description to be used for --help #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/seastar/core/app-template.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,15 @@ private:
std::shared_ptr<smp> _smp;
seastar_options _opts;
boost::program_options::options_description _app_opts;
boost::program_options::options_description* _app_visible_opts_for_help = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest use &_app_opts as the default value of _app_visible_opts_for_help, like:

boost::program_options::options_description* _app_visible_opts_for_help = &_app_opts;

this would help with the readability -- it's clear that we use _app_opts by default, when handling --help.

boost::program_options::options_description _seastar_opts;
boost::program_options::options_description _opts_conf_file;
boost::program_options::positional_options_description _pos_opts;
std::optional<boost::program_options::variables_map> _configuration;
configuration_reader _conf_reader;

configuration_reader get_default_configuration_reader();
void add_extra_options(boost::program_options::options_description& opts);
public:
struct positional_option {
const char* name;
Expand All @@ -153,6 +155,7 @@ public:
boost::program_options::options_description& get_options_description();
boost::program_options::options_description& get_conf_file_options_description();
boost::program_options::options_description_easy_init add_options();
void set_app_visible_options_for_help(boost::program_options::options_description* opts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we really want to accept nullptr as a valid input, probably should add a doxygen comment to explain what nullptr is for. and, mutating the value passed by caller and expect that caller to keep the referenced object alive might also warrant a comment.

still i think a reference is a better fit in this case.

void add_positional_options(std::initializer_list<positional_option> options);
boost::program_options::variables_map& configuration();
int run_deprecated(int ac, char ** av, std::function<void ()>&& func) noexcept;
Expand Down
35 changes: 24 additions & 11 deletions src/core/app-template.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,7 @@ app_template::app_template(app_template::seastar_options opts)
if (!alien::internal::default_instance) {
alien::internal::default_instance = _alien.get();
}
_app_opts.add_options()
("help,h", "show help message")
;
_app_opts.add_options()
("help-seastar", "show help message about seastar options")
;
_app_opts.add_options()
("help-loggers", "print a list of logger names and exit")
;

add_extra_options(_app_opts);
{
program_options::options_description_building_visitor visitor;
_opts.describe(visitor);
Expand All @@ -109,12 +100,33 @@ app_template::app_template(app_template::config cfg)
{
}

void
app_template::add_extra_options(boost::program_options::options_description& opts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of mutating the parameter, i'd suggest returning a boost::program_options::options_description. more readable this way, IMHO. BTW, could make this function static, if we don't want to mark it with the const specifier.

opts.add_options()
("help,h", "show help message")
;
opts.add_options()
("help-seastar", "show help message about seastar options")
;
opts.add_options()
("help-loggers", "print a list of logger names and exit")
;
}

app_template::~app_template() = default;

const app_template::seastar_options& app_template::options() const {
return _opts;
}

void
app_template::set_app_visible_options_for_help(boost::program_options::options_description* opts) {
_app_visible_opts_for_help = opts;
if (_app_visible_opts_for_help) {
add_extra_options(*_app_visible_opts_for_help);
}
}
Comment on lines +124 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be more explicit, like:

if (opts) {
    add_extra_options(*opts);
    _app_visible_opts_for_help = opts;
}

this way, it's clear that we mutate the pointee of opts, and it's a noop, if opts is nullptr. but i'd suggest accept a reference instead. like

set_app_visible_options_for_help(boost::program_options::options_description& opts)

as, why would the caller want to pass a nullptr for a noop?


app_template::configuration_reader app_template::get_default_configuration_reader() {
return [this] (bpo::variables_map& configuration) {
auto home = std::getenv("HOME");
Expand Down Expand Up @@ -211,7 +223,8 @@ app_template::run_deprecated(int ac, char ** av, std::function<void ()>&& func)
if (!_opts.description.empty()) {
std::cout << _opts.description << "\n";
}
std::cout << _app_opts << "\n";
auto& which_opts = _app_visible_opts_for_help ? *_app_visible_opts_for_help : _app_opts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use &_app_opts as the default value of _app_visible_opts_for_help. we could simplify this part like:

        assert(_app_visible_opts_for_help);
        std::cout << *_app_visible_opts_for_help << "\n";

std::cout << which_opts << "\n";
return 0;
}
if (configuration.count("help-seastar")) {
Expand Down