-
Notifications
You must be signed in to change notification settings - Fork 90
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
Command-line help for input options for factory created types #2483
Conversation
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.
clang-tidy made some suggestions
@@ -78,13 +78,16 @@ public: | |||
|
|||
using Factory::create; | |||
ReturnType create(Options* options, int y_offset = 0, Mesh* mesh = nullptr, | |||
Region<Ind3D> region_in = {}) { | |||
Region<Ind3D> region_in = {}) const { |
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.
warning: the parameter region_in
is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Region<Ind3D> region_in = {}) const {
^
const &
return Factory::create(options, y_offset, mesh, region_in); | ||
} | ||
ReturnType create(int y_offset = 0, Mesh* mesh = nullptr, | ||
Region<Ind3D> region_in = {}) { | ||
Region<Ind3D> region_in = {}) const { |
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.
warning: the parameter region_in
is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Region<Ind3D> region_in = {}) const {
^
const &
mesh_options["nz"] = 4; | ||
|
||
// We might need a global mesh for some types, so best make one | ||
bout::globals::mpi = new MpiWrapper(); |
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.
warning: assigning newly created gsl::owner<>
to non-owner MpiWrapper *
[cppcoreguidelines-owning-memory]
bout::globals::mpi = new MpiWrapper();
^
|
||
if (current_arg == help_arg) { | ||
if (i + 1 >= argc) { | ||
throw BoutException(_("Usage is {} {} <name>\n"), argv[0], help_arg); |
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.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
throw BoutException(_("Usage is {} {} <name>\n"), argv[0], help_arg);
^
if (i + 1 >= argc) { | ||
throw BoutException(_("Usage is {} {} <name>\n"), argv[0], help_arg); | ||
} | ||
printTypeOptions<Factory>(argv[i + 1]); |
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.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
printTypeOptions<Factory>(argv[i + 1]);
^
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.
Neat!
Thanks @ZedThree (and happy New Year!). This looks like a great way to get this functionality in a simple way without lots of code changes. The two-stage construction of the solvers is a bit odd, and moving everything into the constructor would be good. I think |
Co-authored-by: johnomotani <john.omotani@ukaea.uk>
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.
👍
Happy new year to you too @bendudson @johnomotani ! I'll have a look at squashing the solver The other QoL missing from this PR is that the arguments must be separated with a space: it would be nice to split on equals too, but then we need to do more parsing of the command line, and at that point I almost want to find a library. |
(Also, this PR is a factor 10 smaller than #2336 😶) |
Replaces #2336 with a much simpler implementation. It does a very similar thing: pass an empty
Options
to a type to see what options it expects, and the associated default values and docstrings, and then print all that information.#2336 tries to avoid actually constructing types, using a companion template class that just reads
Options
, which has some distinct advantages: we only need anOptions
to construct this companion class, so we don't need to set anything else up; we don't need to worry about any preconditions; we don't do any unnecessary work.However, it does also have some glaring downsides: it requires either lots of duplication or ugly indirection; inheritance is painful to deal with; if default values depended on e.g.
Mesh
values, we have to be able to defer setting the real default value somehow.So what we do here is just create a real object instead. Much simpler.
We need to make sure all the factories can take the explicit name of the type we want plus an
Options
, but this is not very much code. We also need to set up a globalMesh
, but making a very small one is pretty cheap.I was worried that there were some types that can only be constructed with >2 processors, but that doesn't seem to be the case.
Also, all the
Solver
s set most of their options in theirinit
functions, which makes them a little bit pointless:If we're happy with the direction of this PR, I'll change types to read the
Options
in their constructors instead.