Skip to content

Conversation

@CaseyCarter
Copy link
Contributor

@CaseyCarter CaseyCarter commented Jun 8, 2021

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.

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
36 tasks
@CaseyCarter CaseyCarter changed the title <fstream>: Implement LWG-3430 <fstream>: Implement LWG-3430 Jun 8, 2021
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
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

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.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LWG Library Working Group issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants