Remove uses of char* in app CLI #2911

daljit46 · 2024-05-21T11:06:10Z

This PR aims to replace uses of C-style const char* arguments with std::string in our CLI. C-style arrays for list of arguments/options are also replaced in favour of std::vector<std::string>. Partially fulfils one of the objectives in #2585 and #2111.

daljit46 · 2024-05-21T11:08:13Z

@Lestropie @jdtournier I will need your help with migrating this part of the code in src/gui/mrview.cpp:721-755:

  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);
  }

Lestropie · 2024-05-22T08:29:24Z

Message ""options must appear after the last argument - see help page for details"" isn't quite right; it's specifically *mrview-specific` options, ie. not standard options, that must appear after all positional arguments and standard options (at least from looking at what the code is doing).

Lestropie · 2024-05-23T01:07:46Z

Decision during yesterday's meeting was that the scope of this PR would be limited to the CLI; while there are uses of char* elsewhere in the code base, it would be better to address those separately in the future.

cmd/connectome2tck.cpp

daljit46 · 2024-05-28T17:43:20Z

Ok so the above snipped in window.cpp has now been rewritten. @Lestropie let me know if the logic still makes sense.

Lestropie · 2024-05-29T02:02:25Z

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.

Find last item that is an argument or standard option
(or an argument that is provided to a standard option)
Find first item that is not an argument or standard option
(ie. it's an mrview-specific option)
If there's at least one instance of both 1 and 2, make sure that position 1 does not exceed position 2.

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.

daljit46 · 2024-05-29T09:06:57Z

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.

That's a good point and I agree. To improve this, we could add a new index variable for ParsedArgument (just like I did for ParsedOption), which keeps track of the position in the raw command line argument list. Alternatively, we can loop over the raw argument list and compare the id of a ParsedOption against the raw arguments.
I don't know which one I prefer.

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.

Yes, but since this change concerns mrview it'd be difficult to verify it with an ordinary bash test. The app may fail to launch since GUI commands may not work on the CI machines (e.g. due to missing GUI libraries). This is a case where having the possibility to add unit tests that verify the correctness of a single function may be useful.

daljit46 · 2024-05-30T14:06:53Z

@Lestropie btw can an option be repeated? In other words, is this syntax valid: command arg1 arg2 -opt1 -opt1?
If not, then I guess this could work:

      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)
      );

jdtournier · 2024-05-31T13:26:13Z

can an option be repeated?

Yes, but not by default - only if it's been labelled with .allow_multiple(). There are quite a few examples of that in the existing commands (both for arguments or options).

Lestropie · 2024-06-03T04:56:48Z

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 opt[0][0]. The first index references one of possibly multiple appearances of that particular command-line option; the second index references one of potentially multiple arguments expected to appear after that command-line option. Good example would be something like mredit -plane: usage; access.

github-actions

clang-tidy made some suggestions

github-actions · 2024-06-06T12:55:18Z

core/app.cpp

@@ -1128,10 +1140,10 @@ void init(int cmdline_argc, const char *const *cmdline_argv) {

  terminal_use_colour = !ProgressBar::set_update_method();

-  argc = cmdline_argc;
-  argv = cmdline_argv;
+  raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc);


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); ^

github-actions · 2024-06-06T12:55:18Z

core/app.cpp

@@ -1128,10 +1140,10 @@

  terminal_use_colour = !ProgressBar::set_update_method();

-  argc = cmdline_argc;
-  argv = cmdline_argv;
+  raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc);


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); ^

github-actions · 2024-06-06T12:55:18Z

core/app.cpp

-  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());


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

NAME = Path::basename(cmdline_argv[0]); ^

github-actions · 2024-06-06T12:55:18Z

src/dwi/tractography/connectome/matrix.cpp

@@ -22,7 +22,7 @@

 namespace MR::DWI::Tractography::Connectome {

-const char *statistics[] = {"sum", "mean", "min", "max", NULL};
+std::vector<std::string> statistics = {"sum", "mean", "min", "max"};


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"}; ^

github-actions

clang-tidy made some suggestions

github-actions · 2024-06-06T13:10:09Z

core/app.cpp

@@ -1128,10 +1140,10 @@ void init(int cmdline_argc, const char *const *cmdline_argv) {

  terminal_use_colour = !ProgressBar::set_update_method();

-  argc = cmdline_argc;
-  argv = cmdline_argv;
+  raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc);


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

raw_arguments_list = std::vector<std::string>(cmdline_argv, cmdline_argv + cmdline_argc); ^

github-actions

clang-tidy made some suggestions

github-actions · 2024-06-06T15:30:26Z

core/app.cpp

-    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() == '.') {


warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

Suggested change

if (isdigit(no_dash_arg.front() != 0) || no_dash_arg.front() == '.') {

if ((isdigit(no_dash_arg.front() != 0) != 0) || no_dash_arg.front() == '.') {

github-actions · 2024-06-06T15:30:26Z

core/app.cpp

-    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() == '.') {


warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]

Suggested change

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() == '.') {

github-actions

clang-tidy made some suggestions

github-actions · 2024-06-07T12:38:42Z

core/app.cpp

+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 ||


warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]

Suggested change

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 ||

Lestropie

Left a few minor comments. Might be worth adding tests for some of the bespoke CLI handling. Otherwise happy to be merged once you're content.

github-actions

clang-tidy made some suggestions

cmd/mrcalc.cpp

core/app.cpp

daljit46 · 2024-06-19T14:40:41Z

Review feedback addressed

Lestropie

Should be good once the C++ CLI failure on Windows is identified.

Edit: Guessing it's this one:

testing_cpp_cli: [ERROR] Expected exactly 0 arguments (2 supplied)
testing_cpp_cli: [ERROR] Usage: testing_cpp_cli
testing_cpp_cli: [ERROR] Yours: testing_cpp_cli �bool false

MSYS2 is breaking either the em or en dash?

daljit46 · 2024-07-08T10:58:28Z

@Lestropie I've spent a lot of time to try to understand why the parsing of utf-8 characters is failing on MSYS2, but without success. I've found that the MSYS2 port of GCC only supports the "C" and "POSIX" locales. Then I thought that the best thing to do would just to call std::setlocale(LC_ALL,"") to try to automatically select the user's preferred locale. This seem to work at first try with a simple test application I wrote, but when trying to parse out dash characters, the program didn't parse them correctly. To make things worse, when running the executables under CTest the detection of UTF-8 characters didn't even work.

I'm not sure what more I could try as the documentation on these matters is rather limited and sparsed all over the internet with no real source of truth.

Lestropie · 2024-07-15T03:18:36Z

If that magnitude of effort invested hasn't yielded a solution, then I suggest that when running under MSYS2 we bypass the tests that verify handling of non-ASCII dashes. It's a nice-to-have to handle cases of people copy&pasting code examples from programs that have auto-bastardised text, but it's not mission critical.

- 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 <robert.smith@florey.edu.au>

In 6286158, as part of #2911, ColourMap::maps was converted from a null-terminated C array to a std::vector. However, the loops iterating over this list weren't updated and were still relying on null-termination causing an invalid memory access. This also resulted mrview crashing on MacOS.

daljit46 added the code quality label May 21, 2024

daljit46 self-assigned this May 21, 2024

daljit46 marked this pull request as draft May 21, 2024 11:08

This comment was marked as outdated.

Sign in to view

daljit46 added the help wanted label May 21, 2024

Lestropie reviewed May 23, 2024

View reviewed changes

cmd/connectome2tck.cpp Show resolved Hide resolved

daljit46 force-pushed the no_char_arguments branch 3 times, most recently from 25736a7 to f0ae136 Compare May 28, 2024 17:42

This comment was marked as outdated.

Sign in to view

daljit46 force-pushed the no_char_arguments branch from f0ae136 to 444edab Compare May 30, 2024 14:49

daljit46 force-pushed the no_char_arguments branch 2 times, most recently from 155c1b4 to ed8fdd6 Compare June 6, 2024 11:16

github-actions bot reviewed Jun 6, 2024

View reviewed changes

daljit46 force-pushed the no_char_arguments branch from ed8fdd6 to ff9502a Compare June 6, 2024 13:45

github-actions bot reviewed Jun 6, 2024

View reviewed changes

daljit46 force-pushed the no_char_arguments branch from ff9502a to f073290 Compare June 7, 2024 10:56

github-actions bot reviewed Jun 7, 2024

View reviewed changes

daljit46 force-pushed the no_char_arguments branch 2 times, most recently from c3eeefe to d73ce09 Compare June 14, 2024 13:29

Lestropie mentioned this pull request Jun 19, 2024

Remove all instance of NULL #2925

Open

3 tasks

Lestropie requested changes Jun 19, 2024

View reviewed changes

github-actions bot reviewed Jun 19, 2024

View reviewed changes

cmd/mrcalc.cpp Show resolved Hide resolved

core/app.cpp Show resolved Hide resolved

daljit46 force-pushed the no_char_arguments branch from 5f16489 to aed7a86 Compare June 19, 2024 14:39

Lestropie approved these changes Jun 20, 2024

View reviewed changes

daljit46 force-pushed the no_char_arguments branch from 516c20c to c2bb1f5 Compare August 2, 2024 11:48

daljit46 and others added 13 commits August 2, 2024 12:50

Use string_view in dash detection/removal

79d4edf

Replace union with std::variant for argument choices

3ab8fd9

Use std::string for app CLI

b0f8018

Use std::string in rest of the code

6286158

Store index of args in raw CL input in ParsedArgument

c21d48d

This change reflects the same idea implemented in ParsedOption

Update copyright script with new APP_CPP_STRING

f30a90d

Fix parsing of arguments in mrcalc

11421f3

Also adds a new test that verifies that mrcalc parsing of arguments behaves as expected when using MRtrix3 standard options for commands.

Fix command line history string construction

4a76d09

Remove unnecessary comment

32babf0

Update error message in mrview CLI

5495bbe

Co-authored-by: Robert Smith <robert.smith@florey.edu.au>

Add further tests for app CLI

032fb56

Skip dash tests on MSYS2

a6ab4e1

daljit46 force-pushed the no_char_arguments branch from c2bb1f5 to a6ab4e1 Compare August 2, 2024 11:53

daljit46 merged commit d3bb1b4 into dev Aug 13, 2024
6 checks passed

daljit46 deleted the no_char_arguments branch August 13, 2024 14:24

daljit46 mentioned this pull request Aug 21, 2024

Fix invalid memory access when reading colour maps #2974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove uses of char* in app CLI #2911

Remove uses of char* in app CLI #2911

daljit46 commented May 21, 2024 •

edited

Loading

daljit46 commented May 21, 2024 •

edited by Lestropie

Loading

This comment was marked as outdated.

Lestropie commented May 22, 2024

Lestropie commented May 23, 2024

daljit46 commented May 28, 2024

This comment was marked as outdated.

Lestropie commented May 29, 2024

daljit46 commented May 29, 2024

daljit46 commented May 30, 2024 •

edited

Loading

jdtournier commented May 31, 2024

Lestropie commented Jun 3, 2024

github-actions bot left a comment

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot left a comment

github-actions bot Jun 6, 2024

github-actions bot left a comment

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot left a comment

github-actions bot Jun 7, 2024

Lestropie left a comment

github-actions bot left a comment

daljit46 commented Jun 19, 2024

Lestropie left a comment •

edited

Loading

daljit46 commented Jul 8, 2024

Lestropie commented Jul 15, 2024

	if (isdigit(no_dash_arg.front() != 0) \|\| no_dash_arg.front() == '.') {
	if ((isdigit(no_dash_arg.front() != 0) != 0) \|\| no_dash_arg.front() == '.') {

	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() == '.') {

	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 \|\|

Remove uses of char* in app CLI #2911

Remove uses of char* in app CLI #2911

Conversation

daljit46 commented May 21, 2024 • edited Loading

daljit46 commented May 21, 2024 • edited by Lestropie Loading

This comment was marked as outdated.

Lestropie commented May 22, 2024

Lestropie commented May 23, 2024

daljit46 commented May 28, 2024

This comment was marked as outdated.

Lestropie commented May 29, 2024

daljit46 commented May 29, 2024

daljit46 commented May 30, 2024 • edited Loading

jdtournier commented May 31, 2024

Lestropie commented Jun 3, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 7, 2024

Choose a reason for hiding this comment

Lestropie left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

daljit46 commented Jun 19, 2024

Lestropie left a comment • edited Loading

Choose a reason for hiding this comment

daljit46 commented Jul 8, 2024

Lestropie commented Jul 15, 2024

daljit46 commented May 21, 2024 •

edited

Loading

daljit46 commented May 21, 2024 •

edited by Lestropie

Loading

daljit46 commented May 30, 2024 •

edited

Loading

Lestropie left a comment •

edited

Loading