No .type_choice(std::vector<std::string>) by Lestropie · Pull Request #3379 · MRtrix3/mrtrix3

Lestropie · 2026-06-03T11:13:36Z

Extension of #3275.
Closes #3283.

The only circumstance in which it would make logical sense to use a std::vector<std::string> rather than an enumeration for a choice-type command-line argument is if literally the only interpretation of the user specification is as a literal string. In any other circumstance, where there is just one instance of an equivalence test, an explicit enumeration makes a lot more sense. I am confident that the latter is the case every time. Therefore I don't think it makes sense for .type_choice(std::vector<std::string>) to persist.

Replace the std::vector<std::string>-based Argument::type_choice overload with the enum template type_choice<Enum>(), converting every command-line choice option to operate on a strongly-typed enumeration parsed via MR::Enum::from_name / App::get_option_choice. The string vectors that previously defined each option's choices are removed in favour of the backing enums. Enumerators that are meaningful internally but must not be user-selectable (such as tck_stat_t's ENDS_CORR) are excluded from the presented choices by a magic_enum customize::enum_name specialisation, which also causes such values to be rejected when supplied, obviating any parallel CLI-only enumeration. Command-line help text and accepted values are preserved throughout. Session prompts: 1. > In class MR::Argument (cpp/core/cmdline_option.h), remove member > function type_choice(const std::vector<std::string> &). For all > resulting compilation failures, substitute with template > type_choice<Enum>(), defining an appropriate enum if one does not > exist, and modify all parsing of that option to operate on the enum. > Remove any std::vector<std::string> used to define the choices, and > re-confirm successful compilation. 2. > 1. Where get_option_value() or a ternary was used to access an enum > choice or set a default, use get_option_choice() instead. > 2. Where the CLI options are only a subset of an enum's states, find > code detrimentally affected (eg. regression to integers); comment > on whether a std::variant over two enumerations would be beneficial. > 3. Make DataType::Identifier's 8-bit values equal each datatype's > internal DataType encoding. 3. > 1. Augment type_choice() with an optional > std::optional<std::initializer_list<Enum>> of elements not to be > presented at the command-line. > 2. Use it to obviate tck_stat_cli_t. > 3. Revert task 3 above, keeping DataType::Identifier sequentially > indexed in the original order so help strings are unaltered. 4. > For points 1 and 2, revisit enum subsets using magic_enum features > rather than a specified exclusion list — better use of the library. > Report how the corresponding code may appear. Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ector<std::string>) - Somewhat unrelated but code came up during review: Change MR::File::Config::get(std::string) to return a std::optional. - Remove some redundant enums in registration code, and collapse unnecessary if branches. - Add code comments identifying enum usages that should be refactored.

Remove the bespoke geometry_index2type() switch and the geometry_string2index() wrapper in the mrview tractography tool, both of which duplicated functionality already provided by the magic_enum library. Index-to-enum conversions now call magic_enum::enum_value, and string-to-enum conversions call MR::Enum::from_name directly at each call site, with the single-line helpers deleted entirely. The geometry combobox is now populated by looping over magic_enum::enum_names so its entry order is guaranteed to track the enumerator order. Session prompts: 1. > Scan HEAD comment for TODO comments in directory > cpp/gui/mrview/tool/tractography/ relating to sub-optimal use of > enumerations. For each, determine the appropriate corresponding > functionality in the magic_enum library and utilise. For any MRtrix > functions that collapse down to a single line of code, delete the > function and instead perform the computation inline in any invoking > code. Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 52. Check the log or trigger a new build to see more.

github-actions · 2026-06-03T11:30:45Z

-  auto opt = get_options("spacing");
-  const avgspace_voxspacing_t spacing =
-      opt.empty() ? SPACING_DEFAULT_VALUE : static_cast<avgspace_voxspacing_t>(static_cast<int>(opt[0][0]));
+  const avgspace_voxspacing_t spacing = get_option_choice<avgspace_voxspacing_t>("spacing", SPACING_DEFAULT_VALUE);


warning: no header providing "MR::App::get_option_choice" is directly included [misc-include-cleaner]

const avgspace_voxspacing_t spacing = get_option_choice<avgspace_voxspacing_t>("spacing", SPACING_DEFAULT_VALUE); ^

github-actions · 2026-06-03T11:30:45Z

    size_t template_option_count = 0;
-    const MR::Interp::interp_type interp =
-        MR::Interp::interp_type(get_option_value("interp", static_cast<ssize_t>(default_interp)));
+    const MR::Interp::interp_type interp = get_option_choice<MR::Interp::interp_type>("interp", default_interp);


warning: no header providing "MR::App::get_option_choice" is directly included [misc-include-cleaner]

const MR::Interp::interp_type interp = get_option_choice<MR::Interp::interp_type>("interp", default_interp); ^

github-actions · 2026-06-03T11:30:45Z

  const space_t space = get_option_choice<space_t>("space", default_space);
-  const MR::Interp::interp_type interp =
-      MR::Interp::interp_type(get_option_value<ssize_t>("interp", static_cast<ssize_t>(default_interp)));
+  const MR::Interp::interp_type interp = get_option_choice<MR::Interp::interp_type>("interp", default_interp);


warning: no header providing "MR::App::get_option_choice" is directly included [misc-include-cleaner]

const MR::Interp::interp_type interp = get_option_choice<MR::Interp::interp_type>("interp", default_interp); ^

github-actions · 2026-06-03T11:30:46Z

-    }
-  }
+  const Registration::LinearMetricType rigid_metric =
+      get_option_choice<Registration::LinearMetricType>("rigid_metric", Registration::Diff);


warning: no header providing "MR::App::get_option_choice" is directly included [misc-include-cleaner]

get_option_choice<Registration::LinearMetricType>("rigid_metric", Registration::Diff); ^

github-actions · 2026-06-03T11:30:46Z

  }

-  std::vector<std::string> fields;
+  std::vector<Stats::field_t> fields;


warning: no header providing "std::vector" is directly included [misc-include-cleaner]

cpp/cmd/mrstats.cpp:17:

+ #include <vector>

github-actions · 2026-06-03T11:30:47Z

    return default_value;
  try {
-    return to<bool>(value);
+    return to<bool>(from_config.value());


warning: no header providing "MR::to" is directly included [misc-include-cleaner]

cpp/core/file/config.cpp:26:

+ #include "mrtrix.h"

github-actions · 2026-06-03T11:30:47Z

    return default_value;
  try {
-    return to<int>(value);
+    return to<int>(from_config.value());


warning: no header providing "MR::to" is directly included [misc-include-cleaner]

return to<int>(from_config.value()); ^

github-actions · 2026-06-03T11:30:47Z

    return default_value;
  try {
-    return to<float>(value);
+    return to<float>(from_config.value());


warning: no header providing "MR::to" is directly included [misc-include-cleaner]

return to<float>(from_config.value()); ^

github-actions · 2026-06-03T11:30:47Z

+  if (!from_config.has_value())
+    return default_value;
+  try {
+    std::vector<default_type> V(parse_floats(from_config.value()));


warning: no header providing "MR::default_type" is directly included [misc-include-cleaner]

std::vector<default_type> V(parse_floats(from_config.value())); ^

github-actions · 2026-06-03T11:30:47Z

+  if (!from_config.has_value())
+    return default_value;
+  try {
+    std::vector<default_type> V(parse_floats(from_config.value()));


warning: no header providing "MR::parse_floats" is directly included [misc-include-cleaner]

std::vector<default_type> V(parse_floats(from_config.value())); ^

github-actions

clang-tidy made some suggestions

github-actions · 2026-06-03T13:37:49Z

+  if (!from_config.has_value())
+    return default_value;
+  try {
+    std::vector<default_type> V(parse_floats(from_config.value()));


warning: no header providing "std::vector" is directly included [misc-include-cleaner]

cpp/core/file/config.cpp:17:

+ #include <vector>

github-actions · 2026-06-03T13:37:50Z

+      throw Exception("malformed RGB entry \"" + from_config.value() + "\" for key \"" + key + "\"" + //
+                      "in configuration file - ignored");
+    return {static_cast<float>(V[0]), static_cast<float>(V[1]), static_cast<float>(V[2])};
+  } catch (Exception) {


warning: catch handler catches by value; should catch by reference instead [misc-throw-by-value-catch-by-reference]

} catch (Exception) { ^

github-actions · 2026-06-03T13:37:50Z


  static void set(std::string_view key, std::string_view value) { config[std::string(key)] = std::string(value); }
-  static std::string get(std::string_view key);
+  static std::optional<std::string> get(std::string_view key);


warning: no header providing "std::optional" is directly included [misc-include-cleaner]

cpp/core/file/config.h:20:

+ #include <optional>

github-actions · 2026-06-03T13:37:50Z

 enum LinearMetricType { Diff, NCC };
 enum LinearRobustMetricEstimatorType { L1, L2, LP, None };
-enum OptimiserAlgoType { bbgd, gd, none };
+enum OptimiserAlgoType { BBGD, GD };


warning: enum 'OptimiserAlgoType' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum OptimiserAlgoType { BBGD, GD }; ^

github-actions · 2026-06-03T13:37:50Z

+// Command-line choice enumerations.
+// The lowercase enumerator names define the set of valid choices for the
+//   corresponding command-line options; see linear.cpp.
+enum class init_translation_t { mass, geometric, none };


warning: enum 'init_translation_t' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum class init_translation_t { mass, geometric, none }; ^

github-actions · 2026-06-03T13:37:51Z

    return;

-  TrackGeometryType geom_type = geometry_index2type(selected_index);
+  TrackGeometryType geom_type = magic_enum::enum_value<TrackGeometryType>(static_cast<size_t>(selected_index));


warning: variable 'geom_type' of type 'TrackGeometryType' can be declared 'const' [misc-const-correctness]

Suggested change

TrackGeometryType geom_type = magic_enum::enum_value<TrackGeometryType>(static_cast<size_t>(selected_index));

TrackGeometryType const geom_type = magic_enum::enum_value<TrackGeometryType>(static_cast<size_t>(selected_index));

github-actions · 2026-06-03T13:37:51Z

+  if (!from_config.has_value())
    return default_key;
-
+  const std::string value = lowercase(from_config.value());


warning: no header providing "MR::lowercase" is directly included [misc-include-cleaner]

cpp/gui/mrview/window.cpp:30:

- #include "mrview/mode/base.h" + #include "mrtrix.h" + #include "mrview/mode/base.h"

github-actions · 2026-06-03T13:37:51Z

+  if (!from_config.has_value())
    return default_key;
-
+  const std::string value = lowercase(from_config.value());


warning: no header providing "std::string" is directly included [misc-include-cleaner]

cpp/gui/mrview/window.cpp:19:

- #include <unordered_map> + #include <string> + #include <unordered_map>

github-actions · 2026-06-03T13:37:51Z

-  if (init_size_string.length())
-    init_window_size = parse_ints<uint32_t>(init_size_string);
+  if (from_config.has_value())
+    init_window_size = parse_ints<uint32_t>(from_config.value());


warning: no header providing "MR::parse_ints" is directly included [misc-include-cleaner]

init_window_size = parse_ints<uint32_t>(from_config.value()); ^

github-actions · 2026-06-03T13:37:51Z

 using namespace App;

-const std::vector<std::string> choices = {"One", "Two", "Three"};
+enum class Choice { One, Two, Three };


warning: enum 'Choice' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum class Choice { One, Two, Three }; ^

github-actions

clang-tidy made some suggestions

github-actions · 2026-06-04T02:06:01Z

      " for a given contrast type given the individual values"
      " from the tracks passing through each voxel;"
-      " options are: " + join(voxel_statistics, ", ") +
+      " options are: " + MR::Enum::join<vox_stat_t>() +


warning: no header providing "MR::DWI::Tractography::Mapping::vox_stat_t" is directly included [misc-include-cleaner]

cpp/cmd/tckdfc.cpp:20:

- #include "enum.h" + #include "dwi/tractography/mapping/twi_stats.h" + #include "enum.h"

Lestropie added 3 commits June 2, 2026 01:08

Lestropie self-assigned this Jun 3, 2026

Lestropie added the code quality label Jun 3, 2026

Lestropie changed the title ~~No type choice stdstring~~ No type.choice(std::vector<std::string>) Jun 3, 2026

Lestropie changed the title ~~No type.choice(std::vector<std::string>)~~ No .type_choice(std::vector<std::string>) Jun 3, 2026

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

Manual fixes to enum adoption #3379

cc5f197

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

Lestropie mentioned this pull request Jun 4, 2026

Force use of enumerations for .type_choice() arguments #3283

Closed

clang-tidy suggestions #3379

37ab55f

github-actions Bot reviewed Jun 4, 2026

View reviewed changes

Lestropie merged commit cae6472 into dev Jun 4, 2026
6 of 7 checks passed

Lestropie deleted the no_type_choice_stdstring branch June 4, 2026 02:06

	TrackGeometryType geom_type = magic_enum::enum_value<TrackGeometryType>(static_cast<size_t>(selected_index));
	TrackGeometryType const geom_type = magic_enum::enum_value<TrackGeometryType>(static_cast<size_t>(selected_index));

Conversation

Lestropie commented Jun 3, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects