Page MenuHomePhabricator

[libcxx] Implement P2467R1: Support exclusive mode for fstreams
Needs ReviewPublic

Authored by PragmaTwice on Nov 8 2022, 6:28 AM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
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!

Diff Detail

Event Timeline

PragmaTwice created this revision.Nov 8 2022, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 6:28 AM
Herald added a subscriber: arichardson. · View Herald Transcript
PragmaTwice requested review of this revision.Nov 8 2022, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 6:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
PragmaTwice edited the summary of this revision. (Show Details)Nov 8 2022, 6:36 AM
PragmaTwice edited the summary of this revision. (Show Details)

apply clang format patch

apply generated docs patch from CI

generate ios.version.compile.pass.cpp

fix conditional inclusion

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

philnik requested changes to this revision.Nov 9 2022, 11:58 AM
philnik added a subscriber: 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
2

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

18
30

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.

55

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

This revision now requires changes to proceed.Nov 9 2022, 11:58 AM

replace defined(__cpp_lib_ios_noreplace) with _LIBCPP_STD_VER >= 23

PragmaTwice marked an inline comment as done.

update test file

PragmaTwice marked 4 inline comments as done.

finish changes requested by philnik, so thanks!

ldionne requested changes to this revision.Nov 20 2022, 11:28 AM
ldionne added a subscriber: 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.

This revision now requires changes to proceed.Nov 20 2022, 11:28 AM
PragmaTwice marked an inline comment as done.

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 added inline comments.Dec 1 2022, 8:12 AM
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 updated this revision to Diff 479547.EditedDec 2 2022, 1:00 AM

Added more tests according to @Idionne 's suggestions

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

philnik accepted this revision as: philnik.Dec 6 2022, 2:07 AM

LGTM. Let's see what @ldionne thinks.

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

rebase to latest main branch and add P2467R1 to release note

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.