-
Notifications
You must be signed in to change notification settings - Fork 47
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
Stricter arguments parsing #406
Conversation
brayns/Brayns.cpp
Outdated
@@ -143,12 +144,20 @@ struct Brayns::Impl : public PluginAPI | |||
<< "' is not a valid Brayns plugin; missing " | |||
"brayns_plugin_create()" | |||
<< std::endl; | |||
return; | |||
exit(0); |
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.
EXIT_FAILURE
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.
(...or throw to have a single exit point below)
brayns/Brayns.cpp
Outdated
Brayns::~Brayns() | ||
{ | ||
} | ||
Brayns::~Brayns() {} |
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.
= default in the header
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.
This needs to be in the cpp file due to the pimpl pattern.
@@ -82,6 +88,8 @@ class ApplicationParameters : public AbstractParameters | |||
_updateValue(_synchronousMode, synchronousMode); | |||
} | |||
|
|||
bool getParallelRendering() { return _parallelRendering; } |
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.
const function
@@ -79,6 +78,7 @@ void ParametersManager::parse(int argc, const char** argv) | |||
catch (po::error& e) | |||
{ | |||
BRAYNS_ERROR << e.what() << std::endl; | |||
exit(0); |
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.
also EXIT_FAILURE
} | ||
|
||
ospInit(&argc, newArgv.data()); | ||
ospInit(&argc, argv.data()); |
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.
for consistency, argv[0] should be still the app name imo. I don't know if ospray needs that internally or expects it to be there.
af700fd
to
add55aa
Compare
Issues fixed and rebased. |
Also remove the passing of all arguments to ospray init.
The plugin arguments are parsed by Brayns and then added as argc, argv variables during the creation of the plugin.
add55aa
to
485a901
Compare
This PR make Brayns exit if invoked with incorrect arguments. It also add the possibility to add arguments to plugins.