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
Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 2,070 ms | libcxx CI Clang-cl (DLL) > llvm-libc++-shared-clangcl-cfg-in.std/input_output/file_streams/fstreams/fstream_cons::noreplace.pass.cpp Script:
--
: 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w3\llvm-project\libcxx-ci\libcxx\test\std\input.output\file.streams\fstreams\fstream.cons\noreplace.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -I C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -I C:/ws/w3/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -nostdlib -L C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-dll/lib -lc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w3\llvm-project\libcxx-ci\build\clang-cl-dll\test\std\input.output\file.streams\fstreams\fstream.cons\Output\noreplace.pass.cpp.dir\t.tmp.exe
| |
| 1,890 ms | libcxx CI Clang-cl (Static) > llvm-libc++-static-clangcl-cfg-in.std/input_output/file_streams/fstreams/fstream_cons::noreplace.pass.cpp Script:
--
: 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w2\llvm-project\libcxx-ci\libcxx\test\std\input.output\file.streams\fstreams\fstream.cons\noreplace.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w2/llvm-project/libcxx-ci/build/clang-cl-static/include/c++/v1 -I C:/ws/w2/llvm-project/libcxx-ci/build/clang-cl-static/include/c++/v1 -I C:/ws/w2/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -llibc++experimental -nostdlib -L C:/ws/w2/llvm-project/libcxx-ci/build/clang-cl-static/lib -llibc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w2\llvm-project\libcxx-ci\build\clang-cl-static\test\std\input.output\file.streams\fstreams\fstream.cons\Output\noreplace.pass.cpp.dir\t.tmp.exe
| |
| 1,850 ms | libcxx CI Clang-cl (no vcruntime exceptions) > llvm-libc++-shared-no-vcruntime-clangcl-cfg-in.std/input_output/file_streams/fstreams/fstream_cons::noreplace.pass.cpp Script:
--
: 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w7\llvm-project\libcxx-ci\libcxx\test\std\input.output\file.streams\fstreams\fstream.cons\noreplace.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/include/c++/v1 -I C:/ws/w7/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0 -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -nostdlib -L C:/ws/w7/llvm-project/libcxx-ci/build/clang-cl-no-vcruntime/lib -lc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w7\llvm-project\libcxx-ci\build\clang-cl-no-vcruntime\test\std\input.output\file.streams\fstreams\fstream.cons\Output\noreplace.pass.cpp.dir\t.tmp.exe
| |
| 2,280 ms | libcxx CI MinGW (DLL, i686) > llvm-libc++-mingw-cfg-in.std/input_output/file_streams/fstreams/fstream_cons::noreplace.pass.cpp Script:
--
: 'COMPILED WITH'; C:/llvm-mingw/bin/i686-w64-mingw32-clang++.exe C:\ws\w8\llvm-project\libcxx-ci\libcxx\test\std\input.output\file.streams\fstreams\fstream.cons\noreplace.pass.cpp --target=i686-w64-windows-gnu -nostdinc++ -I C:/ws/w8/llvm-project/libcxx-ci/build/mingw-dll-i686/include/c++/v1 -I C:/ws/w8/llvm-project/libcxx-ci/build/mingw-dll-i686/include/c++/v1 -I C:/ws/w8/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/w8/llvm-project/libcxx-ci/build/mingw-dll-i686/lib -lc++ -o C:\ws\w8\llvm-project\libcxx-ci\build\mingw-dll-i686\test\std\input.output\file.streams\fstreams\fstream.cons\Output\noreplace.pass.cpp.dir\t.tmp.exe
| |
| 2,000 ms | libcxx CI MinGW (DLL, x86_64) > llvm-libc++-mingw-cfg-in.std/input_output/file_streams/fstreams/fstream_cons::noreplace.pass.cpp Script:
--
: 'COMPILED WITH'; C:/llvm-mingw/bin/x86_64-w64-mingw32-clang++.exe C:\ws\w5\llvm-project\libcxx-ci\libcxx\test\std\input.output\file.streams\fstreams\fstream.cons\noreplace.pass.cpp --target=x86_64-w64-windows-gnu -nostdinc++ -I C:/ws/w5/llvm-project/libcxx-ci/build/mingw-dll/include/c++/v1 -I C:/ws/w5/llvm-project/libcxx-ci/build/mingw-dll/include/c++/v1 -I C:/ws/w5/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/w5/llvm-project/libcxx-ci/build/mingw-dll/lib -lc++ -o C:\ws\w5\llvm-project\libcxx-ci\build\mingw-dll\test\std\input.output\file.streams\fstreams\fstream.cons\Output\noreplace.pass.cpp.dir\t.tmp.exe
| |
| View Full Test Results (8 Failed) |
Event Timeline
Comment Actions
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)? | |
Comment Actions
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. | |
Comment Actions
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! | |
| 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. | |
Comment Actions
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).
Comment Actions
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? |
Comment Actions
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.