Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port from C++11 to C++17 #2585

Open
3 of 6 tasks
Lestropie opened this issue Feb 9, 2023 · 8 comments
Open
3 of 6 tasks

Port from C++11 to C++17 #2585

Lestropie opened this issue Feb 9, 2023 · 8 comments
Labels
Milestone

Comments

@Lestropie
Copy link
Member

Lestropie commented Feb 9, 2023

Believe the primary relevance was #2297 (QT6)?
But there are other features that we would likely want to change across the code base in the process of transitioning compilation.
Many features can just be made use of whenever a contributor feels like it, but others we might want to adopt across the board.
The goal of this issue is to list & discuss those (so feel free to add to the list).

  • Make use of filesystem library
    This may result in reduction or deprecation of core/file/path.h and/or core/file/utils.h
  • Nested namespaces
    This I'd personally like to see adopted across the code base in order to remove unnecessary indentation.
  • std::byte in place of anywhere that a uint8_t is used as a set of bitwise flags
    Datatype comes to mind, but there are likely others.
    Edit: In addition to bitwise flags, there's also the question of pointers to binary data. On looking around a bit, one suggestion seems to be to use std::byte* for pointers to completely unstructured / untyped binary data, whereas if the implication is that the underlying data will have some type but the handling code is type-agnostic, continue to use void*. But we could equally use std::byte* for any type-agnostic pointers to binary memory. Open to thoughts.
  • This may also be the right context in which to address Back-end: Remove all char* / char** usages #2111
    Edit: C++17 also has std::string_view; this should perhaps be used in instances of interfacing with some code dependency that uses C-style strings, but I'd advocate removing C-style strings across the board where we have the control to do so.
  • Remove custom MR::vector<>, revert to std::vector<>
  • Remove custom make_shared<>() and make_unique<>()
@daljit46
Copy link
Member

daljit46 commented May 19, 2023

For the first issue about using the filesystem library, what is our intention? For example, do we intend to completely replace the Path namespace with std::filesystem or do we want to essentially make Path make a wrapper around the filesystem library? There are some functions in Path that wouldn't be available in std::filesystem.
Also we should think about whether we want to replace existing instances of std::string with std::filesystem::path.

@Lestropie
Copy link
Member Author

Personally, I would want:

  • Any existing usage of std::string referring to a filesystem path to be changed to std::filesystem::path as far upstream as possible, up to and including the type conversion of command-line arguments.
    (Though counter-intuitively there would need to be caution here with respect to image paths due to multi-file numbered image support.
  • To remove MR::Path entirely, and remove as much of the MR::File functionality in core/file/utils.h as possible.
    If it's the case that there are certain functions for which there are not standard replacements, we'll need to wait to see what that list looks like and decide where to put it.

@daljit46
Copy link
Member

daljit46 commented Jun 6, 2023

Can we also get rid of our custom MR::make_unique?

@Lestropie
Copy link
Member Author

Yep MR::make_unique<> should disappear along with MR::make_shared<> (will amend OP)

@Lestropie
Copy link
Member Author

@MRtrix3/mrtrix3-devs: Since there will need to be an explicit rebasing process to generate a 3.1.0 containing contents of dev excluding the cmake changes, and this group of changes is incomplete, do we want to keep 3.1.0 at C++11?

@daljit46
Copy link
Member

My opinion here is that a transition to use C++17 features should not require using a predefined set of C++17 features before switching. I don't really see any benefits in this approach.

So I would say that building with C++17 has no real downsides, even if you don't use any C++17 feature.

@Lestropie
Copy link
Member Author

Only potential downside that comes to mind would be lack of availability of C++17 compiler support for some user attempting to compile from source. I've no idea what the compiler support timelines look like. But if fewer people are compiling from source post production release, perhaps it's safe enough.

@Lestropie
Copy link
Member Author

  • std::filesystem adoption should includes command-line arguments that may be user-specified filesystem paths. Verification of porting such should be facilitated by 8065803 (part of Python CLI changes #2678).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants