[libc++] Implement LWG3430 disallow implicit conversion of the source arguments to `std::filesystem::path` when constructing `std::basic_*fstream` #85079

yronglin · 2024-03-13T13:53:13Z

Implement LWG3430.

… from string_view Signed-off-by: yronglin <yronglin777@gmail.com>

llvmbot · 2024-03-13T13:53:44Z

@llvm/pr-subscribers-libcxx

Author: None (yronglin)

Changes

Implement LWG3430.

Full diff: https://github.com/llvm/llvm-project/pull/85079.diff

5 Files Affected:

(modified) libcxx/docs/Status/Cxx23Issues.csv (+1-1)
(modified) libcxx/include/fstream (+10-9)
(modified) libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp (+7)
(modified) libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp (+5)
(modified) libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp (+5)

diff --git a/libcxx/docs/Status/Cxx23Issues.csv b/libcxx/docs/Status/Cxx23Issues.csv
index e00345533b865d..87f60fa5278e70 100644
--- a/libcxx/docs/Status/Cxx23Issues.csv
+++ b/libcxx/docs/Status/Cxx23Issues.csv
@@ -64,7 +64,7 @@
 `2818 <https://wg21.link/LWG2818>`__,"``::std::`` everywhere rule needs tweaking","June 2021","|Nothing To Do|",""
 `2997 <https://wg21.link/LWG2997>`__,"LWG 491 and the specification of ``{forward_,}list::unique``","June 2021","",""
 `3410 <https://wg21.link/LWG3410>`__,"``lexicographical_compare_three_way`` is overspecified","June 2021","|Complete|","17.0","|spaceship|"
-`3430 <https://wg21.link/LWG3430>`__,"``std::fstream`` & co. should be constructible from string_view","June 2021","",""
+`3430 <https://wg21.link/LWG3430>`__,"``std::fstream`` & co. should be constructible from string_view","June 2021","|Complete|","19.0",""
 `3462 <https://wg21.link/LWG3462>`__,"§[formatter.requirements]: Formatter requirements forbid use of ``fc.arg()``","June 2021","|Nothing To Do|","","|format|"
 `3481 <https://wg21.link/LWG3481>`__,"``viewable_range`` mishandles lvalue move-only views","June 2021","Superseded by `P2415R2 <https://wg21.link/P2415R2>`__","","|ranges|"
 `3506 <https://wg21.link/LWG3506>`__,"Missing allocator-extended constructors for ``priority_queue``","June 2021","|Complete|","14.0"
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index 776641b347e6c1..5932a42e1b5392 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -78,8 +78,7 @@ public:
     basic_ifstream();
     explicit basic_ifstream(const char* s, ios_base::openmode mode = ios_base::in);
     explicit basic_ifstream(const string& s, ios_base::openmode mode = ios_base::in);
-    explicit basic_ifstream(const filesystem::path& p,
-                            ios_base::openmode mode = ios_base::in); // C++17
+    template<class T> explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17
     basic_ifstream(basic_ifstream&& rhs);
 
     basic_ifstream& operator=(basic_ifstream&& rhs);
@@ -117,8 +116,7 @@ public:
     basic_ofstream();
     explicit basic_ofstream(const char* s, ios_base::openmode mode = ios_base::out);
     explicit basic_ofstream(const string& s, ios_base::openmode mode = ios_base::out);
-    explicit basic_ofstream(const filesystem::path& p,
-                            ios_base::openmode mode = ios_base::out); // C++17
+    template<class T> explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17
     basic_ofstream(basic_ofstream&& rhs);
 
     basic_ofstream& operator=(basic_ofstream&& rhs);
@@ -158,8 +156,8 @@ public:
     basic_fstream();
     explicit basic_fstream(const char* s, ios_base::openmode mode = ios_base::in|ios_base::out);
     explicit basic_fstream(const string& s, ios_base::openmode mode = ios_base::in|ios_base::out);
-    explicit basic_fstream(const filesystem::path& p,
-                           ios_base::openmode mode = ios_base::in|ios_base::out); C++17
+    template<class T>
+    explicit basic_fstream(const T& s, ios_base::openmode mode = ios_base::in | ios_base::out); // Since C++17
     basic_fstream(basic_fstream&& rhs);
 
     basic_fstream& operator=(basic_fstream&& rhs);
@@ -1043,8 +1041,9 @@ public:
 #  endif
   _LIBCPP_HIDE_FROM_ABI explicit basic_ifstream(const string& __s, ios_base::openmode __mode = ios_base::in);
 #  if _LIBCPP_STD_VER >= 17
+  template <class _Tp, std::enable_if_t<std::is_same_v<_Tp, std::filesystem::path>>* = nullptr>
   _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY _LIBCPP_HIDE_FROM_ABI explicit basic_ifstream(
-      const filesystem::path& __p, ios_base::openmode __mode = ios_base::in)
+      const _Tp& __p, ios_base::openmode __mode = ios_base::in)
       : basic_ifstream(__p.c_str(), __mode) {}
 #  endif // _LIBCPP_STD_VER >= 17
   _LIBCPP_HIDE_FROM_ABI basic_ifstream(basic_ifstream&& __rhs);
@@ -1197,8 +1196,9 @@ public:
   _LIBCPP_HIDE_FROM_ABI explicit basic_ofstream(const string& __s, ios_base::openmode __mode = ios_base::out);
 
 #  if _LIBCPP_STD_VER >= 17
+  template <class _Tp, std::enable_if_t<std::is_same_v<_Tp, std::filesystem::path>>* = nullptr>
   _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY _LIBCPP_HIDE_FROM_ABI explicit basic_ofstream(
-      const filesystem::path& __p, ios_base::openmode __mode = ios_base::out)
+      const _Tp& __p, ios_base::openmode __mode = ios_base::out)
       : basic_ofstream(__p.c_str(), __mode) {}
 #  endif // _LIBCPP_STD_VER >= 17
 
@@ -1356,8 +1356,9 @@ public:
                                                ios_base::openmode __mode = ios_base::in | ios_base::out);
 
 #  if _LIBCPP_STD_VER >= 17
+  template <class _Tp, std::enable_if_t<std::is_same_v<_Tp, std::filesystem::path>>* = nullptr>
   _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY _LIBCPP_HIDE_FROM_ABI explicit basic_fstream(
-      const filesystem::path& __p, ios_base::openmode __mode = ios_base::in | ios_base::out)
+      const _Tp& __p, ios_base::openmode __mode = ios_base::in | ios_base::out)
       : basic_fstream(__p.c_str(), __mode) {}
 #  endif // _LIBCPP_STD_VER >= 17
 
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp
index bc75d04740da02..e0e211b4478a7d 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp
@@ -21,11 +21,18 @@
 #include <fstream>
 #include <filesystem>
 #include <cassert>
+#include <type_traits>
+
 #include "test_macros.h"
 #include "platform_support.h"
 
 namespace fs = std::filesystem;
 
+struct fake_path {};
+
+static_assert(std::is_constructible_v<std::fstream, fs::path>);
+static_assert(!std::is_constructible_v<std::fstream, fake_path>);
+
 int main(int, char**) {
   fs::path p = get_temp_file_name();
   {
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
index cfbb8419fe1c5b..72030c2983e4b3 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
@@ -29,6 +29,11 @@
 
 namespace fs = std::filesystem;
 
+struct fake_path {};
+
+static_assert(std::is_constructible_v<std::ifstream, fs::path>);
+static_assert(!std::is_constructible_v<std::ifstream, fake_path>);
+
 int main(int, char**) {
   {
     fs::path p;
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp
index 316ed776a48b54..01c90f326a130b 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp
@@ -27,6 +27,11 @@
 
 namespace fs = std::filesystem;
 
+struct fake_path {};
+
+static_assert(std::is_constructible_v<std::ofstream, fs::path>);
+static_assert(!std::is_constructible_v<std::ofstream, fake_path>);
+
 int main(int, char**) {
   fs::path p = get_temp_file_name();
   {

xiaoyang-sde · 2024-03-13T22:44:39Z

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

+struct fake_path {};
+
+static_assert(std::is_constructible_v<std::fstream, fs::path>);
+static_assert(!std::is_constructible_v<std::fstream, fake_path>);


It seems that these code compiles even without this pull request because fake_path can't be converted to filesystem::path: https://godbolt.org/z/1dcsjdeEb

Good catch! And thanks for your review! I've refined the test cases, also the title need update.

Note: the issue description is now a misnomer, as adding a string_view constructor is no longer being considered at this time.

Signed-off-by: yronglin <yronglin777@gmail.com>

github-actions · 2024-03-14T13:22:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Signed-off-by: yronglin <yronglin777@gmail.com>

mordante

This LWG issue does not what the title says. Based on the issue

 This means the simpler, more obvious code is slower and uses more memory:

    string_view sv = "foo.txt";
    fstream f1(sv); // bad
    fstream f2(string(sv)); // good

We should just allow passing a string_view directly, since it already compiles but doesn't do what anybody expects or wants.

The accepted resolution avoids the path to string_view conversion, but doesn't add the string_view constructor overload. Based on https://godbolt.org/z/TqoWTxT75 the change would disallow using a string_view instead of making its usage more efficient. I wonder whether this change in behaviour is intended, especially since the title suggests differently and the LWG-issue doesn't change Annex C. @jwakely is the change in behaviour intended?

@yronglin let's put this patch on hold for now.

mordante · 2024-03-16T14:10:33Z

libcxx/include/fstream

@@ -78,8 +78,7 @@ public:
    basic_ifstream();
    explicit basic_ifstream(const char* s, ios_base::openmode mode = ios_base::in);
    explicit basic_ifstream(const string& s, ios_base::openmode mode = ios_base::in);
-    explicit basic_ifstream(const filesystem::path& p,
-                            ios_base::openmode mode = ios_base::in); // C++17
+    template<class T> explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17


Suggested change

template<class T> explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17

template<class T>

explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17

mordante · 2024-03-16T14:10:47Z

libcxx/include/fstream

@@ -117,8 +116,7 @@ public:
    basic_ofstream();
    explicit basic_ofstream(const char* s, ios_base::openmode mode = ios_base::out);
    explicit basic_ofstream(const string& s, ios_base::openmode mode = ios_base::out);
-    explicit basic_ofstream(const filesystem::path& p,
-                            ios_base::openmode mode = ios_base::out); // C++17
+    template<class T> explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17


Suggested change

template<class T> explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17

template<class T>

explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17

libcxx/include/fstream

mordante · 2024-03-16T14:17:17Z

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

+static_assert(test_non_convert_to_path<wchar_t>());
+#endif // !TEST_HAS_NO_WIDE_CHARACTERS && !_LIBCPP_HAS_OPEN_WITH_WCHAR
+
+#if TEST_STD_VER > 17 && defined(__cpp_char8_t)


Suggested change

#if TEST_STD_VER > 17 && defined(__cpp_char8_t)

#ifndef TEST_HAS_NO_CHAR8_T

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

jwakely · 2024-03-16T18:19:14Z

The accepted resolution avoids the path to string_view conversion, but doesn't add the string_view constructor overload. Based on https://godbolt.org/z/TqoWTxT75 the change would disallow using a string_view instead of making its usage more efficient. I wonder whether this change in behaviour is intended, especially since the title suggests differently and the LWG-issue doesn't change Annex C. @jwakely is the change in behaviour intended?

Yes, that was what LWG suggested and LEWG approved.

mordante · 2024-03-16T19:13:24Z

The accepted resolution avoids the path to string_view conversion, but doesn't add the string_view constructor overload. Based on https://godbolt.org/z/TqoWTxT75 the change would disallow using a string_view instead of making its usage more efficient. I wonder whether this change in behaviour is intended, especially since the title suggests differently and the LWG-issue doesn't change Annex C. @jwakely is the change in behaviour intended?

Yes, that was what LWG suggested and LEWG approved.

Thanks for the confirmation.

mordante · 2024-03-16T19:15:10Z

@yronglin Can you mention this change in behavior as a breaking change in the release notes.

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2024-03-17T15:28:03Z

@yronglin Can you mention this change in behavior as a breaking change in the release notes.

Thanks for your review! Sure, I've added a release notes item.

Signed-off-by: yronglin <yronglin777@gmail.com>

mordante

This looks quite good. For now I'm happy to have this without a "old behaviour switch", but I've asked Louis whether he can test it internally to see whether this breaks things.

mordante · 2024-03-17T17:28:31Z

libcxx/docs/ReleaseNotes/19.rst

@@ -97,7 +97,7 @@ TODO

 ABI Affecting Changes
 ---------------------
-TODO
+ - LWG3430: Disallow implicit conversion of the source arguments to ``std::filesystem::path`` when constructing ``std::basic_*fstream``.


I think this is better placed under Deprecations and Removals.

This text is technically correct. However the target audience of the release notes are typical developers and not people who read LWG issues and papers. So let's improve this text by explaining what it does. For example,

This effectively removes the possibility to directly construct a ``std::basic_fstream`` from a ``std::basic_stringview`` or a C-string, instead you can construct a temporary ``std::basic_string``. This change has been applied to C++17 and later.

Feel free to write this in your own words.

Thanks, I've update release notes wording.

ldionne · 2024-03-18T01:55:04Z

@mordante let me know about this and suggested it could be interesting to test on a large code base to see whether "removing" the string_view constructor creates some breakage. Please ping me once this is ready to try out and I can kick that off.

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2024-03-18T11:23:34Z

@mordante let me know about this and suggested it could be interesting to test on a large code base to see whether "removing" the string_view constructor creates some breakage. Please ping me once this is ready to try out and I can kick that off.

Sure!

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2024-03-22T10:22:34Z

friendly ping~

yronglin · 2024-03-28T10:56:48Z

ping～

ldionne

I'll take this for a run and let you know next week.

Edit: note to self 125553528

ldionne · 2024-03-28T12:02:11Z

libcxx/docs/ReleaseNotes/19.rst

+  constructing ``std::basic_*fstream``. This effectively removes the possibility to directly construct 
+  a ``std::basic_*fstream`` from a ``std::basic_string_view``, a input-iterator or a C-string, instead 


You have trailing whitespace in here.

Good catch! I've removed that.

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2024-03-28T12:49:55Z

I'll take this for a run and let you know next week.

Thanks for your review! Awaiting your good news.

ldionne · 2024-04-02T13:48:54Z

I am seeing a bit of breakage of the exact form we'd expect:

error: no matching constructor for initialization of 'std::ifstream' (aka 'basic_ifstream<char>')
In file included from [...]:
[...]/usr/include/c++/v1/__fwd/fstream.h:24:28: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'std::string_view' (aka 'basic_string_view<char>') to 'const ifstream' for 1st argument

However, this breakage seems surmountable (a few projects broke but not that many). It'll be slightly annoying but not a deal breaker.

After reading the LWG issue, my understanding is that the intended usage is to instead construct a std::string explicitly when passing it to the std::fstream so as to explicitly call out that we're creating a null-terminated string. Is that correct?

Pinging @llvm/libcxx-vendors for awareness but this LGTM.

jwakely · 2024-04-02T13:58:12Z

After reading the LWG issue, my understanding is that the intended usage is to instead construct a std::string explicitly when passing it to the std::fstream so as to explicitly call out that we're creating a null-terminated string. Is that correct?

Yes. That was the intended usage, which was then broken by adding filesystem::path which allowed an implicit (but expensive) conversion.

mordante

Thanks for testing the patch internally @ldionne!

LGTM modulo one minor issue. I'd like to have a quick look after that is resolved.

libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin · 2024-04-05T17:54:18Z

Seems the CI issue was not caused by this PR

EricWF

LGTM once the bots come back.

yronglin · 2024-04-05T18:01:32Z

Thanks for your review!

yronglin · 2024-04-06T12:34:35Z

CI has green and I merged this PR.

mordante · 2024-04-07T13:57:25Z

CI has green and I merged this PR.

LGTM. Next time when somebody likes to review it before committing, please give them some time to look at the changes.

yronglin · 2024-04-07T14:02:01Z

CI has green and I merged this PR.

LGTM. Next time when somebody likes to review it before committing, please give them some time to look at the changes.

Thank you for the review and help, I will do this next time.

nico · 2024-04-11T17:15:01Z

This breaks existing code, e.g. this:

  std::fstream model_file(ml_package_dir()
                              .Append(kMlPackageDataDir)
                              .Append(kMlPackageModelFileName)
                              .value(),
                          std::ios::out | std::ios::binary);

.value() returns a std::wstring. (This is a Windows-only thing, which is probably why @ldionne didn't encounter it.)

Is the assumption that this will be rarely hit in practice?

full diag text

stderr:
../../services/webnn/coreml/graph_builder.cc(586,16): error: no matching constructor for initialization of 'std::fstream' (aka 'basic_fstream<char>')
  586 |   std::fstream model_file(ml_package_dir()
      |                ^          ~~~~~~~~~~~~~~~~
  587 |                               .Append(kMlPackageDataDir)
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~
  588 |                               .Append(kMlPackageModelFileName)
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  589 |                               .value(),
      |                               ~~~~~~~~~
  590 |                           std::ios::out | std::ios::binary);
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/libc++/src/include\fstream(1411,34): note: candidate constructor not viable: no known conversion from 'const StringType' (aka 'const basic_string<wchar_t>') to 'const char *' for 1st argument
 1411 |   _LIBCPP_HIDE_FROM_ABI explicit basic_fstream(const char* __s,
      |                                  ^             ~~~~~~~~~~~~~~~
../../third_party/libc++/src/include\fstream(1414,34): note: candidate constructor not viable: no known conversion from 'const StringType' (aka 'const basic_string<wchar_t>') to 'const wchar_t *' for 1st argument
 1414 |   _LIBCPP_HIDE_FROM_ABI explicit basic_fstream(const wchar_t* __s,
      |                                  ^             ~~~~~~~~~~~~~~~~~~
../../third_party/libc++/src/include\fstream(1417,34): note: candidate constructor not viable: no known conversion from 'const basic_string<wchar_t>' to 'const basic_string<char>' for 1st argument
 1417 |   _LIBCPP_HIDE_FROM_ABI explicit basic_fstream(const string& __s,
      |                                  ^             ~~~~~~~~~~~~~~~~~
../../third_party/libc++/src/include\fstream(1422,74): note: candidate template ignored: requirement 'is_same_v<std::wstring, std::filesystem::path>' was not satisfied [with _Tp = StringType]
 1422 |   _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY _LIBCPP_HIDE_FROM_ABI explicit basic_fstream(
      |                                                                          ^
../../third_party/libc++/src/include\fstream(1427,25): note: candidate constructor not viable: requires single argument '__rhs', but 2 arguments were provided
 1427 |   _LIBCPP_HIDE_FROM_ABI basic_fstream(basic_fstream&& __rhs);
      |                         ^             ~~~~~~~~~~~~~~~~~~~~~
../../third_party/libc++/src/include\__fwd/fstream.h(28,28): note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
   28 | class _LIBCPP_TEMPLATE_VIS basic_fstream;
      |                            ^~~~~~~~~~~~~
../../third_party/libc++/src/include\fstream(1410,25): note: candidate constructor not viable: requires 0 arguments, but 2 were provided
 1410 |   _LIBCPP_HIDE_FROM_ABI basic_fstream();
      |                         ^

(https://source.chromium.org/chromium/chromium/src/+/main:services/webnn/coreml/graph_builder.cc;l=586?q=services%2Fwebnn%2Fcoreml%2Fgraph_builder.cc)

nico · 2024-04-11T17:51:50Z

libcxx/include/fstream

-    explicit basic_fstream(const filesystem::path& p,
-                           ios_base::openmode mode = ios_base::in|ios_base::out); C++17
+    template<class T>
+    explicit basic_fstream(const T& s, ios_base::openmode mode = ios_base::in | ios_base::out); // Since C++17


Actually, https://en.cppreference.com/w/cpp/io/basic_fstream/basic_fstream mentions a const std::filesystem::path::value_type* overload which kinda happened to work before this change (since it'd construct a ::path and call that ctor), but this change breaks that. Should we undo this change here until an explicit const std::filesystem::path::value_type* ctor exists?

That constructor already exists:

llvm-project/libcxx/include/fstream

Lines 1413 to 1415 in a417b9b

# ifdef _LIBCPP_HAS_OPEN_WITH_WCHAR

_LIBCPP_HIDE_FROM_ABI explicit basic_fstream(const wchar_t* __s,

ios_base::openmode __mode = ios_base::in | ios_base::out);

It's definitely expected that you can't open an fstream with a wstring, the constructor taking a filesystem::path should not be used to convert "anything string-like, with any character type, in any encoding" into a file name with implicit allocations and character conversions.

If you want that conversion, you can still do it explicitly:

std::fstream model_file(filesystem::path(ml_package_dir() .Append(kMlPackageDataDir) .Append(kMlPackageModelFileName) .value()), std::ios::out | std::ios::binary);

Oh, I suppose I need to call c_str() on my wstring to get the wchar_t ctor. Thanks, let me try that; sorry for the noise.

(The point stands that this breaks existing code, though.)

It's indeed correct this may break existing code. Louis tested this at Apple, based on the results we applied the possibly breaking change. Is the breakage for you worse?

It looks like it's just one file so far (). I got a bit confused because the wchar_t ctor is missing from the synopsis comment at the top. So all good.

*: But actually rolling this in is blocked on #86843 (comment) , so not 100% sure yet. But very likely that's all.

[libc++] Implement LWG3430 std::fstream & co. should be constructible…

50656d0

… from string_view Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin requested a review from a team as a code owner March 13, 2024 13:53

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 13, 2024

xiaoyang-sde reviewed Mar 13, 2024

View reviewed changes

[libc++] Refine test cases

5e995e9

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin requested review from ldionne, mordante and philnik777 March 14, 2024 13:22

format

3090674

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin changed the title ~~[libc++] Implement LWG3430 std::fstream & co. should be constructible from string_view~~ [libc++] Implement LWG3430 avoid implicit conversion of the source arguments to std::filesystem::path when constructing std::basic_*fstream Mar 14, 2024

yronglin added 3 commits March 15, 2024 01:30

Fix test

0116f4a

Signed-off-by: yronglin <yronglin777@gmail.com>

Fix test

af85858

Signed-off-by: yronglin <yronglin777@gmail.com>

Refine code

44b09c5

Signed-off-by: yronglin <yronglin777@gmail.com>

mordante requested changes Mar 16, 2024

View reviewed changes

yronglin added 2 commits March 17, 2024 23:04

Merge branch 'main' into LWG3430

41aab57

Address review comments and add release notes

eca5675

Signed-off-by: yronglin <yronglin777@gmail.com>

Add period

2babb84

Signed-off-by: yronglin <yronglin777@gmail.com>

mordante reviewed Mar 17, 2024

View reviewed changes

Update release notes

fe70853

Signed-off-by: yronglin <yronglin777@gmail.com>

yronglin added 3 commits March 18, 2024 19:24

Update release notes

79a0973

Signed-off-by: yronglin <yronglin777@gmail.com>

Revert unnecessery changes

e59fcb2

Signed-off-by: yronglin <yronglin777@gmail.com>

Revert unnecessery changes

8da6092

Signed-off-by: yronglin <yronglin777@gmail.com>

ldionne reviewed Mar 28, 2024

View reviewed changes

Remove trailing whitespace

bae96cf

Signed-off-by: yronglin <yronglin777@gmail.com>

mordante requested changes Apr 5, 2024

View reviewed changes

libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp Outdated Show resolved Hide resolved

yronglin added 3 commits April 6, 2024 01:35

Use TEST_HAS_OPEN_WITH_WCHAR replace _LIBCPP_HAS_OPEN_WITH_WCHAR

917f988

Signed-off-by: yronglin <yronglin777@gmail.com>

Merge branch 'main' into LWG3430

1e073cd

format

aaad833

Signed-off-by: yronglin <yronglin777@gmail.com>

EricWF approved these changes Apr 5, 2024

View reviewed changes

Merge branch 'main' into LWG3430

616c732

yronglin requested a review from mordante April 6, 2024 06:27

yronglin merged commit 4761e74 into llvm:main Apr 6, 2024
55 checks passed

nico reviewed Apr 11, 2024

View reviewed changes

	template<class T> explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17
	template<class T>
	explicit basic_ifstream(const T& s, ios_base::openmode mode = ios_base::in); // Since C++17

	template<class T> explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17
	template<class T>
	explicit basic_ofstream(const T& s, ios_base::openmode mode = ios_base::out); // Since C++17

	#if TEST_STD_VER > 17 && defined(__cpp_char8_t)
	#ifndef TEST_HAS_NO_CHAR8_T

		constructing ``std::basic_*fstream``. This effectively removes the possibility to directly construct
		a ``std::basic_*fstream`` from a ``std::basic_string_view``, a input-iterator or a C-string, instead

	# ifdef _LIBCPP_HAS_OPEN_WITH_WCHAR
	_LIBCPP_HIDE_FROM_ABI explicit basic_fstream(const wchar_t* __s,
	ios_base::openmode __mode = ios_base::in \| ios_base::out);

[libc++] Implement LWG3430 disallow implicit conversion of the source arguments to std::filesystem::path when constructing std::basic_*fstream #85079

[libc++] Implement LWG3430 disallow implicit conversion of the source arguments to std::filesystem::path when constructing std::basic_*fstream #85079

Conversation

yronglin commented Mar 13, 2024

llvmbot commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2024 • edited Loading

mordante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwakely commented Mar 16, 2024

mordante commented Mar 16, 2024

mordante commented Mar 16, 2024

yronglin commented Mar 17, 2024 • edited Loading

mordante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne commented Mar 18, 2024 • edited Loading

yronglin commented Mar 18, 2024

yronglin commented Mar 22, 2024

yronglin commented Mar 28, 2024

ldionne left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yronglin Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

yronglin commented Mar 28, 2024

ldionne commented Apr 2, 2024

jwakely commented Apr 2, 2024

mordante left a comment

Choose a reason for hiding this comment

yronglin commented Apr 5, 2024

EricWF left a comment

Choose a reason for hiding this comment

yronglin commented Apr 5, 2024

yronglin commented Apr 6, 2024

mordante commented Apr 7, 2024

yronglin commented Apr 7, 2024

nico commented Apr 11, 2024

Choose a reason for hiding this comment

jwakely Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nico Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[libc++] Implement LWG3430 disallow implicit conversion of the source arguments to `std::filesystem::path` when constructing `std::basic_*fstream` #85079

[libc++] Implement LWG3430 disallow implicit conversion of the source arguments to `std::filesystem::path` when constructing `std::basic_*fstream` #85079

github-actions bot commented Mar 14, 2024 •

edited

Loading

yronglin commented Mar 17, 2024 •

edited

Loading

ldionne commented Mar 18, 2024 •

edited

Loading

ldionne left a comment •

edited

Loading

yronglin Mar 28, 2024 •

edited

Loading

jwakely Apr 11, 2024 •

edited

Loading

nico Apr 11, 2024 •

edited

Loading