This patch brings std::ios_base::noreplace from P2467R1 to libc++.
This requires compiling the shared library in C++23 mode since otherwise
fstream::open(...) doesn't know about the new flag.
Details
- Reviewers
philnik Mordante PragmaTwice ldionne - Group Reviewers
Restricted Project - Commits
- rGa3f17ba3febb: [libc++] Implement P2467R1: Support exclusive mode for fstreams
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for working on this! Please goo through https://libcxx.llvm.org/Contributing.html#pre-commit-check-list and make sure that you did everything listed there.
libcxx/include/fstream | ||
---|---|---|
557 | We don't want anybody to think that this is configurable. Same for the other places. | |
libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp | ||
1 ↗ | (On Diff #474143) | It seems like you are missing tests for everything but in | out | trunc | noreplace. Please add them. |
17 ↗ | (On Diff #474143) | |
29 ↗ | (On Diff #474143) | We normally avoid this to make it simpler for readers to see where symbols are coming from. In this case you are using fs in a bunch of different places too, making it even harder to read. It also looks like there is just a single use of fs:: anywhere, so it doesn't even remove any verbosity. |
54 ↗ | (On Diff #474143) | Why aren't you using std::filesystem::remove(p)? |
Thanks for the patch! This looks reasonable but I think we're missing coverage.
libcxx/include/ios | ||
---|---|---|
61 | ||
libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp | ||
1 ↗ | (On Diff #474458) | This should be inside fstream.cons/path.pass.cpp instead. And we're missing tests for the other places where this flag can now be used, like basic_filebuf, basic_ofstream, etc. Those tests should be added to their existing tests for constructors that take openmode. |
add since C++23 comment
libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp | ||
---|---|---|
1 ↗ | (On Diff #474458) | Hi @ldionne, thanks for your review. I notice that there is a comment UNSUPPORTED: c++03, c++11, c++14 in path.pass.cpp, but it seems we need UNSUPPORTED: c++03, c++11, c++14, c++17, c++20 for noreplace. Hence I wonder if a new test file is more suitable here? Thanks! |
libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp | ||
---|---|---|
1 ↗ | (On Diff #474458) | You can guard the new test with TEST_STD_VER >= 23. |
Please also add a release note to libcxx/docs/ReleaseNotes.rst under the Implemented Papers section and make sure CI is green (you probably have to rebase).
I didn't do a review, I was mainly curious what this paper did. I did discover one small issue.
libcxx/docs/ReleaseNotes.rst | ||
---|---|---|
52 ↗ | (On Diff #481838) | Can you update libcxx/docs/Status/Cxx2bPapers.csv` too? |
Thanks for adding test coverage! This looks good to me, however we have several CI failures that would need to be fixed. It looks like UBSan, Windows and a few others are not happy with this change as-is.
@PragmaTwice we're moving to GitHub PRs it would be great when this patch can be finished before moving. Are you interested in finishing it? If not I'll take over. Please start by rebasing the patch to see whether it passes the CI.
I'll XFAIL the few remaining tests on AIX and ping the AIX folks to get some support. The rest looks good so I'm shipping this.
We don't want anybody to think that this is configurable. Same for the other places.