Skip to content

Add supporting std::filesystem::path#42

Merged
metayeti merged 3 commits intometayeti:masterfrom
adamgene:wip/adam/support_filesystem_path
Mar 30, 2025
Merged

Add supporting std::filesystem::path#42
metayeti merged 3 commits intometayeti:masterfrom
adamgene:wip/adam/support_filesystem_path

Conversation

@adamgene
Copy link
Contributor

@adamgene adamgene commented Feb 21, 2025

Support std::filesystem::path, also add testing and cmake. I tested in Ubuntu 24.04 with gcc 13, also 20.04 with gcc 9.4.0 and 8.X. I already use this simplest change from 0.9.10, and used in VS2017~2022/macOS.

@adamgene
Copy link
Contributor Author

I am not sure which formater should use, It's fine to re-formating by you.

@adamgene
Copy link
Contributor Author

@metayeti Could you check these changes? I want to discuss another approach. As you can see, path is used in fstream. Is it just necessary to allow the caller to inject the corresponding fstream object into the class that uses it? This can preserve the original use of string and expand the use of fstream; the externally injected fstream can decide whether to support the use of path.

@metayeti
Copy link
Owner

Hey. This looks proper, thanks for the PR - I will most likely merge it. The only things missing I think is updating version numbers and the changelog - but I can do those. I'll review by the end of the week in more detail

1. to force writing utf8bom into test file. Because Windows can not support unicode in char.
2. merge tests to verify read/write.
@adamgene
Copy link
Contributor Author

I tested on both Windows and macOS, and made changes to fix some errors.

@metayeti metayeti merged commit 10aa5e4 into metayeti:master Mar 30, 2025
@metayeti
Copy link
Owner

Merged, ty for the contribution! Also sorry you had to wait a bit.

@GXTX
Copy link

GXTX commented May 17, 2025

ftr this ups the requires C++ version to 17 from like 11 or 98...

@metayeti
Copy link
Owner

ftr this ups the requires C++ version to 17 from like 11 or 98...

True, I didn't really think about that. Doesn't bother me personally but I can see how it may be an issue for some.

@adamgene
Copy link
Contributor Author

You can use last mINI 0.9.17 that still uses std::string w/o c++17. Maybe add a warning in README.md?

ftr this ups the requires C++ version to 17 from like 11 or 98...

True, I didn't really think about that. Doesn't bother me personally but I can see how it may be an issue for some.

@metayeti
Copy link
Owner

I'll add a note to releases for now. I may retroactively add one in readme at a next update

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants