Skip to content

<fstream>: Implement LWG-3430#1968

Merged
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
CaseyCarter:lwg3430
Jun 29, 2021
Merged

<fstream>: Implement LWG-3430#1968
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
CaseyCarter:lwg3430

Conversation

@CaseyCarter
Copy link
Contributor

@CaseyCarter CaseyCarter commented Jun 8, 2021 •
edited
Loading

LWG-3430 turns the constructors of basic_{i,o,}fstream that accept filesystem::path into templates that deduce the argument type and then constrain it to be filesystem::path to avoid potentially expensive implicit conversions. We already implement these constructors as templates for esoteric ABI-compatibility reasons (See

STL/stl/inc/fstream

Lines 20 to 25 in 4c862ee

// TRANSITION, ABI: The _Path_ish functions accepting filesystem::path or experimental::filesystem::path are templates
// which always use the same types as a workaround for user code deriving from iostreams types and
// __declspec(dllexport)ing the derived types. Adding member functions to iostreams broke the ABI of such DLLs.
// Deriving and __declspec(dllexport)ing standard library types is not supported, but in this particular case
// the workaround was inexpensive. The workaround will be removed in the next ABI breaking release of the
// Visual C++ Libraries.
) so the net change here is to make that template deduce the argument type and constrain it. I think we should also change our experimental::filesystem::path constructors in the spirit of LWG-3430, so I merged pairs of experimental / non-experimental constructors together using a new _Is_any_path trait to constrain.

Existing test coverage in tests\std\tests\VSO_0000000_path_stream_parameter seems sufficient, so I've added no new tests.

@CaseyCarter
<fstream>: Implement LWG-3430
1ac546d 
LWG-3430 turns the constructors of `basic_{i,o,}fstream` that accept `filesystem::path` into templates that deduce the argument type and then constrain it to be `filesystem::path` to avoid potentially expensive implicit conversions. We already implement these constructors as templates for esoteric ABI-compatibility reasons (See https://github.com/microsoft/STL/blob/4c862ee11ce956556b810a813e77b0f8f97fb642/stl/inc/fstream#L20-L25) so the net change here is to make that template deduce the argument type and constrain it. I think we should also change our `experimental::filesystem::path` constructors in the spirit of LWG-3430, so I merged pairs of `experimental` / non-`experimental` constructors together using a new `_Is_any_path` trait to constrain

Existing test coverage in `tests\std\tests\VSO_0000000_path_stream_parameter` seems sufficient, so I've added no new tests.
@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Jun 8, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner June 8, 2021 17:01
@CaseyCarter CaseyCarter mentioned this pull request Jun 8, 2021
Closed
36 tasks
@CaseyCarter CaseyCarter changed the title <fstream>: Implement LWG-3430 <fstream>: Implement LWG-3430 Jun 8, 2021
miscco
miscco approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Thats much better

@barcharcraz barcharcraz self-assigned this Jun 16, 2021
barcharcraz
barcharcraz approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

This could break user code right? Since with our current implementation we'd accept any type with a .c_str() overload.

@CaseyCarter
Copy link
Contributor Author

CaseyCarter commented Jun 21, 2021

This could break user code right? Since with our current implementation we'd accept any type with a .c_str() overload.

Since the _Path_ish template parameters were only used in non-deduced contexts, and there's no way to pass explicit template arguments to a constructor, there was previously no way for a user to pass an arbitrary type with a c_str member.

This will break user code that was constructing from a type that converts to path, but that is intentional breakage per LWG-3430.

StephanTLavavej
StephanTLavavej approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good - I eventually realized that the other _Path_ish occurrences for open() can't be changed like this (as tempting as it would be).

@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2021
@StephanTLavavej StephanTLavavej merged commit 4449d8a into microsoft:main Jun 29, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 29, 2021

Thanks for implementing this LWG issue resolution that also simplifies the code! 😺 📉 🎉

@CaseyCarter CaseyCarter deleted the lwg3430 branch June 29, 2021 18:29
@jedieaston jedieaston mentioned this pull request Nov 17, 2021
Merged
2 tasks
@StephanTLavavej StephanTLavavej mentioned this pull request Feb 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@barcharcraz barcharcraz barcharcraz approved these changes

@miscco miscco miscco approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

LWG Library Working Group issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CaseyCarter @StephanTLavavej @barcharcraz @miscco