-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
WIP: Use std filesystem path #4562
base: master
Are you sure you want to change the base?
WIP: Use std filesystem path #4562
Conversation
#ifndef ITK_FUTURE_LEGACY_REMOVE | ||
const_cast<std::string &>(m_FileName) = filepath.string(); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that this const_cast<std::string &>(m_FileName) = filepath.string()
is undefined behavior, when m_FileName
is defined by const std::string m_FileName{}
. The compiler is allowed to assume that m_FileName
is never modified, when it is declared like that.
You could work around it by declaring m_FileName
as a const reference to a non-const string, instead:
const std::string & m_FileName = m_NonConstFileName;
#ifndef ITK_FUTURE_LEGACY_REMOVE | ||
/** Filename to read */ | ||
std::string m_FileName{}; | ||
const std::string m_FileName{}; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be preferable to make m_FileName
private, instead of protected. Then the workaround (const_cast<std::string &>
) would not be necessary at all. ImageIOBase has a GetFileName and a SetFileName anyway, so direct access to m_FileName
should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_FileName is directly access by all ImageIO and it not trivial to make the change to use the GetFileName accessor. The end goal is to use the GetFilePath accessor, so the idea to leave the compatible "m_FileName" and switch to "GetFilePath".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work is quite welcome! But perhaps wait with this potentially disruptive change until 5.4 is released?
|
||
namespace itk | ||
{ | ||
using PathType = std::filesystem::path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the need for this indirection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding a similar type in SimpleITK to transition from std::string to std::filesystem::path which is being conditioned to the preprocessor (maybe). It also made wrapping with SWIG a little easier.
It is shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would prefer to just use std::filesystem::path
. (Unless it would need to be configurable, as in SimpleITK.) ITK already has so many typedefs (or type aliases, if you like). When doing code analysis, I often lose time, searching for the definition of a typedef. It doesn't always make the code more readable to have all those typedefs.
@blowekamp Very interesting, thanks! I definitely like having a separate type for file paths, rather than just "std::string", for improved type safety. I'm also curious to see if it does improve non-ASCII character support in file names, on Windows. |
Add Set/Get FilePath member functions to ImageIOBase, with the future plan to replace the m_FileName member variable with these access member functions. The m_FileName is directly access by derived ImageIO classes. The m_FileName is now "constant" to disallow derived classes from directly writing to the variable. The SetFilePath method updates the legacy const m_FileName to maintain compatibility.
f541394
to
4c6d087
Compare
It looks like adopting Even though C++17 compilers exist on older version of macOS, the STL shipped on older versions does not support That would suck for me, I still support back to 10.13. |
Our Python packages also require 10.11. |
Add std::filesystem::path to ImageIOBase for eventual wchar support for paths.
This proposes an approach to remove direct access to m_FileName, with the eventual end goal to use GetFilePath and "legacy" GetFileName methods.