Skip to content

When wchar_t is a real type, users shouldn't see unsigned short machinery#2164

Merged
StephanTLavavej merged 14 commits intomicrosoft:mainfrom
fsb4000:fix216
Jun 16, 2022
Merged

When wchar_t is a real type, users shouldn't see unsigned short machinery#2164
StephanTLavavej merged 14 commits intomicrosoft:mainfrom
fsb4000:fix216

Conversation

@fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Aug 28, 2021

Fixes #216

@fsb4000 fsb4000 requested a review from a team as a code owner August 28, 2021 08:16
@fsb4000 fsb4000 marked this pull request as draft August 28, 2021 09:39
@fsb4000
Copy link
Contributor Author

fsb4000 commented Aug 28, 2021

I'm not quite sure if I need to do something with codecvt <unsigned short, char, mbstate_t>, need I?
And to which file do I need to move:

STL/stl/inc/iosfwd

Lines 254 to 258 in 78ff461

#if defined(_CRTBLD)
using ushistream = basic_istream<unsigned short, char_traits<unsigned short>>;
using ushostream = basic_ostream<unsigned short, char_traits<unsigned short>>;
using ushfilebuf = basic_filebuf<unsigned short, char_traits<unsigned short>>;
#endif // defined(_CRTBLD)

?

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 30, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 30, 2021

I'm not quite sure if I need to do something with codecvt <unsigned short, char, mbstate_t>, need I?

I suspect (70% sure) that it should be guarded by #ifdef _CRTBLD instead of #ifdef _NATIVE_WCHAR_T_DEFINED. That way, it will still be exported, but users with native wchar_t won't see it.

STL/stl/inc/xlocale

Lines 2106 to 2108 in 3136525

#ifdef _NATIVE_WCHAR_T_DEFINED
template <>
class _CRTIMP2_PURE_IMPORT codecvt<unsigned short, char, mbstate_t> : public codecvt_base {

STL/stl/inc/xlocale

Line 2306 in 3136525

#endif // _NATIVE_WCHAR_T_DEFINED

And to which file do I need to move:

Only the 5 following source files mention ush* - I am 98% sure that these are the only files that need the typedefs.

STL/stl/src/ushcerr.cpp

Lines 10 to 12 in 3136525

#define wistream ushistream
#define wostream ushostream
#define wfilebuf ushfilebuf

STL/stl/src/ushcin.cpp

Lines 10 to 12 in 3136525

#define wistream ushistream
#define wostream ushostream
#define wfilebuf ushfilebuf

STL/stl/src/ushclog.cpp

Lines 10 to 12 in 3136525

#define wistream ushistream
#define wostream ushostream
#define wfilebuf ushfilebuf

STL/stl/src/ushcout.cpp

Lines 10 to 12 in 3136525

#define wistream ushistream
#define wostream ushostream
#define wfilebuf ushfilebuf

STL/stl/src/ushiostr.cpp

Lines 9 to 11 in 3136525

#define wistream ushistream
#define wostream ushostream
#define wfilebuf ushfilebuf

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Aug 31, 2021 •
edited
Loading

I'm not quite sure if I need to do something with codecvt <unsigned short, char, mbstate_t>, need I?

I suspect (70% sure) that it should be guarded by #ifdef _CRTBLD instead of #ifdef _NATIVE_WCHAR_T_DEFINED. That way, it will still be exported, but users with native wchar_t won't see it.

Summarizing here a discussion I had with myself in Discord: explicit template specializations should be visible whenever it's possible to instantiate them implicitly from a primary template or partial specialization. Hiding explicit specializations is simply asking for ODR violation nightmares. Consequently, I think codecvt <unsigned short, char, mbstate_t> needs to be visible only when _ENFORCE_FACET_SPECIALIZATIONS is 0 (which should be the case when _CRTBLD is defined).

@CaseyCarter CaseyCarter reopened this Aug 31, 2021
@fsb4000 fsb4000 marked this pull request as ready for review September 4, 2021 04:09
@CaseyCarter CaseyCarter self-assigned this Sep 8, 2021
StephanTLavavej
StephanTLavavej requested changes Jun 9, 2022
View reviewed changes
@StephanTLavavej StephanTLavavej self-assigned this Jun 9, 2022
CaseyCarter
CaseyCarter approved these changes Jun 11, 2022
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Thank you for helping to partially excise this historical abomination.

stl/inc/fstream
_CRTIMP2_PURE FILE* __CLRCALL_PURE_OR_CDECL _Fiopen(const char*, ios_base::openmode, int);
_CRTIMP2_PURE FILE* __CLRCALL_PURE_OR_CDECL _Fiopen(const wchar_t*, ios_base::openmode, int);

#ifdef _NATIVE_WCHAR_T_DEFINED
Copy link
Contributor

@CaseyCarter CaseyCarter Jun 11, 2022

Choose a reason for hiding this comment

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

I've never given it any thought before, but seriously who put defined in the name of a macro so we have to query if MEOW_DEFINED is defined??!? This is even worse that putting not in the name of a boolean condition. (No change requested.)

stl/src/ushcerr.cpp
_STD_END

#ifndef wistream
#define wistream ushistream
Copy link
Contributor

@CaseyCarter CaseyCarter Jun 11, 2022

Choose a reason for hiding this comment

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

🤮🤮🤮 (No change requested; looking in these old sources can be painful.)

@CaseyCarter CaseyCarter removed their assignment Jun 11, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 15, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 15, 2022

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 85274ba into microsoft:main Jun 16, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 16, 2022

Thanks again for eliminating these bizarre specializations as much as possible! 👻 🎉 😸

@fsb4000 fsb4000 deleted the fix216 branch June 16, 2022 04:01
fsb4000 added a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@fsb4000
When wchar_t is a real type, users shouldn't see unsigned short machi…
ada7de0 
…nery (microsoft#2164)
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

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

enhancement Something can be improved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

STL: When wchar_t is a real type, users shouldn't see unsigned short machinery

3 participants

@fsb4000 @StephanTLavavej @CaseyCarter