Skip to content

Introduce magic_enum for enum <--> string conversion#3275

Open
daljit46 wants to merge 21 commits intodevfrom
magic_enum
Open

Introduce magic_enum for enum <--> string conversion#3275
daljit46 wants to merge 21 commits intodevfrom
magic_enum

Conversation

@daljit46
Copy link
Member

This PR addresses the issue raised in #3264. It adds magic_enum, a C++17 header-only library for enum to string conversions, as a dependency. The changes include the addition of new templated wrapper functions to perform enum <--> string in mrtrix.h. As an initial step (and to limit the scope of this PR), I have replaced several existing string-based usages with enums. These changes are limited to cpp/cmd; no code in cpp/core is affected at this stage, although that could be considered in a future PR.
One issue I encountered is that some existing string values begin with a number. Since C++ identifiers cannot start with a digit, cases such as "2d" and "3d" in mrdegibbs cannot be mapped directly to enum names. Handling these cases would require either custom parsing logic or changes to the CLI options. I have not made any changes for those cases in this PR.

@daljit46 daljit46 self-assigned this Mar 16, 2026
@daljit46 daljit46 requested a review from a team March 16, 2026 15:50
Copy link

@github-actions github-actions bot left a 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

Copy link

@github-actions github-actions bot left a 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

Copy link

@github-actions github-actions bot left a 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

@Lestropie
Copy link
Member

Looks nice 👍 Left a few small suggestions.

cases such as "2d" and "3d" in mrdegibbs

Some cases could be dealt with via interface design rather than software engineering. Eg. This functionality could easily be changed from -mode as .type_choice() to -dimensions as .type_integer(2, 4). You didn't happen to keep a list?

@daljit46
Copy link
Member Author

You didn't happen to keep a list?

Going through all the commands, the only other two places where I saw this problem were: peaksconvert.cpp and mtnormalise.cpp.

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member

Proposal to change command-line interfaces for better compatibility with magic_enum:

  • mrdegibbs: Change -mode from .type_choice({"2d", "3d"}) to -dimensionalityas.type_integer(2, 4). Option is only present on dev, not present in 3.0.x.
  • peaksconvert: Very new command that basically nobody other than me knows or cares about. Change options:
    "unitspherical", "spherical", "unit3vector", "3vector"
    to one of:
    "unitspherical", "spherical", "unitthreevector", "threevector"
    "unitspherical", "spherical", "uniteuclidean", "euclidean"
    (alternative suggestions welcome)
  • mtnormalise: Should be easy to change from .type_choice({"0", "1", "2", "3"}) to .type_integer(0, 4).

Given the list is small, changes are non-intrusive, and it would facilitate making .type_choice() work exclusively on enums, I think we just go ahead with this.

@daljit46
Copy link
Member Author

(alternative suggestions welcome)

How about "unitspherical", "spherical", "unitcartesian", "cartesian"?

@Lestropie
Copy link
Member

That's probably what I subconsciously meant, but my conscious somehow landed on the wrong word 😅

@daljit46
Copy link
Member Author

Bikeshedding question: lots of places in the CLI options now use join_enum to convert enum lists to strings. The default separator is a comma with no space in between (e.g. choice1,choice2,choice3). Do we want to add a space?

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member

Somewhat inconsistent across the code base. I'd probably opt for including a space myself: it's intended for human reading, not parsing (anything wanting to parse that information should instead be invoking __print_full_usage__).

@daljit46
Copy link
Member Author

All feedback has been addressed, documentation rebuilt to follow CLI changes and all tests are now passing. This should be ready to be merged.

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member

Thoughts on removing
Argument &type_choice(const std::vector<std::string> &c)
in favour of forcing use of
template <typename Enum> Argument &type_choice()?
Personally I would prefer that if there are no contraindicated use cases.

@daljit46
Copy link
Member Author

Yes, that is the end result I would like to see too. From a quick grep, I can see there are still 26 instances of using the old overload of type_choices() . It's obviously possible in principles to replace all those instances with the enum-templated version, but I think we should do so in a future PR. The changes made here are already touching a lot of files and I would prefer to not extend the scope of this PR even further. I think that's best strategy for avoiding introduction of unwarranted issues (despite the trivial nature of the changes here).

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants