Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
50656d0
[libc++] Implement LWG3430 std::fstream & co. should be constructible…
yronglin Mar 13, 2024
5e995e9
[libc++] Refine test cases
yronglin Mar 14, 2024
3090674
format
yronglin Mar 14, 2024
0116f4a
Fix test
yronglin Mar 14, 2024
af85858
Fix test
yronglin Mar 15, 2024
44b09c5
Refine code
yronglin Mar 15, 2024
41aab57
Merge branch 'main' into LWG3430
yronglin Mar 17, 2024
eca5675
Address review comments and add release notes
yronglin Mar 17, 2024
2babb84
Add period
yronglin Mar 17, 2024
fe70853
Update release notes
yronglin Mar 18, 2024
79a0973
Update release notes
yronglin Mar 18, 2024
e59fcb2
Revert unnecessery changes
yronglin Mar 18, 2024
8da6092
Revert unnecessery changes
yronglin Mar 18, 2024
bae96cf
Remove trailing whitespace
yronglin Mar 28, 2024
917f988
Use TEST_HAS_OPEN_WITH_WCHAR replace _LIBCPP_HAS_OPEN_WITH_WCHAR
yronglin Apr 5, 2024
1e073cd
Merge branch 'main' into LWG3430
yronglin Apr 5, 2024
aaad833
format
yronglin Apr 5, 2024
616c732
Merge branch 'main' into LWG3430
yronglin Apr 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcxx/docs/Status/Cxx23Issues.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
19 changes: 10 additions & 9 deletions libcxx/include/fstream
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

basic_ifstream(basic_ifstream&& rhs);

basic_ifstream& operator=(basic_ifstream&& rhs);
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

basic_ofstream(basic_ofstream&& rhs);

basic_ofstream& operator=(basic_ofstream&& rhs);
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jwakely jwakely Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nico nico Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

basic_fstream(basic_fstream&& rhs);

basic_fstream& operator=(basic_fstream&& rhs);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/path.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


int main(int, char**) {
fs::path p = get_temp_file_name();
{
Expand Down
5 changes: 5 additions & 0 deletions libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions libcxx/test/std/input.output/file.streams/fstreams/ofstream.cons/path.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
{
Expand Down