[libcxx] Implement P2467R1: Support exclusive mode for fstreams

[PragmaTwice]

Summary

This patch brings std::ios_base::noreplace in P2467R1 to libcxx.

Since I am new to the community, I do not quite know who is suitable to review my patch, so if there is better choice as reviewers in your mind, please add to "reviewers". So thanks!

[PragmaTwice]

apply clang format patch

[PragmaTwice]

apply generated docs patch from CI

[PragmaTwice]

generate ios.version.compile.pass.cpp

[PragmaTwice]

fix conditional inclusion

[PragmaTwice]

replace path.c_str() with path.string().c_str()

[philnik]

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

It seems like you are missing tests for everything but in | out | trunc | noreplace. Please add them.

17

29

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

Why aren't you using std::filesystem::remove(p)?

[PragmaTwice]

replace defined(__cpp_lib_ios_noreplace) with _LIBCPP_STD_VER >= 23

[PragmaTwice]

rebase

[PragmaTwice]

update test file

[PragmaTwice]

finish changes requested by philnik, so thanks!

[ldionne]

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

2

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.

[PragmaTwice]

add since C++23 comment

libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp

2

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!

[philnik]

libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp

2

You can guard the new test with TEST_STD_VER >= 23.

[PragmaTwice]

Added more tests according to @Idionne 's suggestions

[PragmaTwice]

Hi @ldionne @philnik , more tests are added now, feel free to review it again. So thanks!

[philnik]

LGTM. Let's see what @ldionne thinks.

[philnik]

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).

[PragmaTwice]

rebase to latest main branch and add P2467R1 to release note

[Mordante]

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?

[ldionne]

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.