This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2467R1: Support exclusive mode for fstreams
ClosedPublic

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

Details

Summary

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.

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
1

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
1

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
1

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.

Mordante requested changes to this revision.Sep 4 2023, 9:51 AM

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

This revision now requires changes to proceed.Sep 4 2023, 9:51 AM
ldionne commandeered this revision.Sep 18 2023, 2:13 PM
ldionne edited reviewers, added: PragmaTwice; removed: ldionne.

[Github PR Transition cleanup]

Commandeering to finish.

ldionne updated this revision to Diff 556977.Sep 18 2023, 3:15 PM
ldionne retitled this revision from [libcxx] Implement P2467R1: Support exclusive mode for fstreams to [libc++] Implement P2467R1: Support exclusive mode for fstreams.
ldionne edited the summary of this revision. (Show Details)

Rebase, fix tests. In particular, this patch requires building the dylib with C++23.

Hi @Mordante @ldionne, sorry for the delayed response.

Is there anything I can assist with now, or it is ready for merging?

Hi @Mordante @ldionne, sorry for the delayed response.

Is there anything I can assist with now, or it is ready for merging?

No, I think this is almost ready to go now.

ldionne accepted this revision.Fri, Nov 17, 2:25 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Nov 17, 2:27 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.