Remove uses of char* in app CLI

[daljit46]

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]

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

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]

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.

[daljit46]

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

[Lestropie]

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.

1. Find last item that is an argument or standard option (or an argument that is provided to a standard option)
2. Find first item that is not an argument or standard option (ie. it's an mrview-specific option)
3. 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]

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]

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

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]

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.

[daljit46]

This is now ready for review. I fixed all the mistakes that were causing the tests to fail and also did some manual testing to check if things behaved as expected. @Lestropie @jdtournier can you take a look at this?

[daljit46]

Review feedback addressed