Conversation
|
@Lestropie @jdtournier I will need your help with migrating this part of the code in if (!MR::App::argument.empty()) {
if (!MR::App::option.empty()) {
// check that first non-standard option appears after last argument:
size_t last_arg_pos = 1;
for (; MR::App::argv[last_arg_pos] != MR::App::argument.back().c_str(); ++last_arg_pos)
if (MR::App::argv[last_arg_pos] == nullptr)
throw Exception("FIXME: error determining position of last argument!");
// identify first non-standard option:
size_t first_option = 0;
for (; first_option < MR::App::option.size(); ++first_option) {
if (size_t(MR::App::option[first_option].opt - &MR::App::__standard_options[0]) >=
MR::App::__standard_options.size())
break;
}
if (MR::App::option.size() > first_option) {
first_option = MR::App::option[first_option].args - MR::App::argv;
if (first_option < last_arg_pos)
throw Exception("options must appear after the last argument - see help page for details");
}
}
std::vector<std::unique_ptr<MR::Header>> list;
for (size_t n = 0; n < MR::App::argument.size(); ++n) {
try {
list.push_back(std::make_unique<MR::Header>(MR::Header::open(MR::App::argument[n])));
} catch (CancelException &e) {
for (const auto &msg : e.description)
CONSOLE(msg);
} catch (Exception &e) {
e.display();
}
}
add_images(list);
} |
|
Message " |
|
Decision during yesterday's meeting was that the scope of this PR would be limited to the CLI; while there are uses of |
25736a7 to
f0ae136
Compare
|
Ok so the above snipped in |
|
It seems counter-intuitive to me that you'd use iterator offsets to interrogate one argument position, but then rely on upstream internal storage of argument position of each command-line option to interrogate another argument position. I would have thought that if you need to determine two different command-line argument positions, the same mechanism should be used for both.
Ultimately, whether this does as intended is best verified by adding command tests, with both valid and invalid uses, and make sure that the former succeeds and the latter fails with the intended error message. |
That's a good point and I agree. To improve this, we could add a new
Yes, but since this change concerns |
|
@Lestropie btw can an option be repeated? In other words, is this syntax valid: const auto first_non_standard_option_pos = std::distance(
MR::App::raw_arguments_list.begin(),
std::find(MR::App::raw_arguments_list.begin(),
MR::App::raw_arguments_list.end(),
first_non_standard_option->opt->id)
); |
f0ae136 to
444edab
Compare
Yes, but not by default - only if it's been labelled with |
|
Also, with respect to the option repeating: you'll sometimes see the value of an argument passed via a command-line option accessed as something like |
155c1b4 to
ed8fdd6
Compare
|
|
||
| argc = cmdline_argc; | ||
| argv = cmdline_argv; | ||
| raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
raw_arguments_list = std::vector<std::string>(cmdline_argv + 1, cmdline_argv + cmdline_argc);
^|
|
||
| argc = cmdline_argc; | ||
| argv = cmdline_argv; | ||
| raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
raw_arguments_list = std::vector<std::string>(cmdline_argv + 1, cmdline_argv + cmdline_argc);
^| argv = cmdline_argv; | ||
| raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc); | ||
| NAME = Path::basename(raw_arguments_list.front()); | ||
| raw_arguments_list.erase(raw_arguments_list.begin()); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
NAME = Path::basename(cmdline_argv[0]);
^| namespace MR::DWI::Tractography::Connectome { | ||
|
|
||
| const char *statistics[] = {"sum", "mean", "min", "max", NULL}; | ||
| std::vector<std::string> statistics = {"sum", "mean", "min", "max"}; |
There was a problem hiding this comment.
warning: variable 'statistics' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::vector<std::string> statistics = {"sum", "mean", "min", "max"};
^|
|
||
| argc = cmdline_argc; | ||
| argv = cmdline_argv; | ||
| raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc);
^ed8fdd6 to
ff9502a
Compare
core/app.cpp
Outdated
| for (size_t i = 0; i < OPTIONS.size(); ++i) | ||
| get_matches(candidates, OPTIONS[i], root); | ||
| get_matches(candidates, __standard_options, root); | ||
| if (isdigit(no_dash_arg.front() != 0) || no_dash_arg.front() == '.') { |
There was a problem hiding this comment.
warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]
| if (isdigit(no_dash_arg.front() != 0) || no_dash_arg.front() == '.') { | |
| if ((isdigit(no_dash_arg.front() != 0) != 0) || no_dash_arg.front() == '.') { |
core/app.cpp
Outdated
| for (size_t i = 0; i < OPTIONS.size(); ++i) | ||
| get_matches(candidates, OPTIONS[i], root); | ||
| get_matches(candidates, __standard_options, root); | ||
| if (isdigit(no_dash_arg.front() != 0) || no_dash_arg.front() == '.') { |
There was a problem hiding this comment.
warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
| if (isdigit(no_dash_arg.front() != 0) || no_dash_arg.front() == '.') { | |
| if (isdigit(static_cast<int>(no_dash_arg.front() != 0)) || no_dash_arg.front() == '.') { |
ff9502a to
f073290
Compare
core/app.cpp
Outdated
| const Option *match_option(std::string_view arg) { | ||
| // let's check if arg starts with a dash | ||
| auto no_dash_arg = without_leading_dash(arg); | ||
| if (arg.size() == no_dash_arg.size() || no_dash_arg.empty() || isdigit(no_dash_arg.front() != 0) != 0 || |
There was a problem hiding this comment.
warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
| if (arg.size() == no_dash_arg.size() || no_dash_arg.empty() || isdigit(no_dash_arg.front() != 0) != 0 || | |
| if (arg.size() == no_dash_arg.size() || no_dash_arg.empty() || isdigit(static_cast<int>(no_dash_arg.front() != 0)) != 0 || |
c3eeefe to
d73ce09
Compare
- Logic has been rewritten using STL algorithms. Not that previously we were using pointer arithmetic to achieve this, which was technically more efficient but less readable and safe. However, performance is not a concern considering the data size involved. - A new `index` member has been added to ParsedOption to identify an option's position in the list of raw command-line arguments list.
This change reflects the same idea implemented in ParsedOption
Also adds a new test that verifies that mrcalc parsing of arguments behaves as expected when using MRtrix3 standard options for commands.
Co-authored-by: Robert Smith <[email protected]>
c2bb1f5 to
a6ab4e1
Compare
This logic mistake was introduced in #2911. Previously, the behaviour was to return `id != nullptr` where id was a `const char*`, but this was mistakenly changed to return `id.empty()` with `id` being a `std::string`. This effectively reversed the logic.
- Stop usage of C-style string data and functions; can be viewed as an exhaustive extension of #2911. - Ensure that "#pragma one" is used instead of the historical "#ifndef __headerpath__", as introduced in #2871. - Enforce use of nested namespaces, as first introduced in #2652. - Enforce explicit resolution of MR::abs() for templated data types; where data types are exclusively scalar floating-point, std::fabs() is instead used. - Preclude use of C-style casts, as proposed in #3010. - Add to MR namespace "NaNF" and "InfF" as floating-point variants of the already-defined default_type "NaN" and "Inf", and preclude use of macros "NAN" and "INFINITY" in favour of either one of those or std::numeric_limits<>, as proposed in #3192. - Preclude use of "NULL" macro, replacing with nullptr.
This PR aims to replace uses of C-style
const char*arguments withstd::stringin our CLI. C-style arrays for list of arguments/options are also replaced in favour ofstd::vector<std::string>. Partially fulfils one of the objectives in #2585 and #2111.