Skip to content
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.

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

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

Merged
merged 14 commits into from
Jun 16, 2022

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

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 {

#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

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

@CaseyCarter
Copy link
Member

CaseyCarter commented Aug 31, 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.

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
stl/inc/fstream Outdated Show resolved Hide resolved
stl/inc/xlocale Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Jun 9, 2022
Copy link
Member

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

@@ -63,9 +63,9 @@ _INLINE_VAR constexpr bool _Is_any_path = _Is_any_of_v<_Ty
_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
Member

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

using ushfilebuf = basic_filebuf<unsigned short, char_traits<unsigned short>>;

_STD_END

#ifndef wistream
#define wistream ushistream
Copy link
Member

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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
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