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

codecvt: no conversion for all char-like types of size 1. #2739

Merged
merged 10 commits into from
Jun 2, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented May 25, 2022

Fixes #2109
Fixes #817

@fsb4000 fsb4000 requested a review from a team as a code owner May 25, 2022 11:28
@fsb4000
Copy link
Contributor Author

fsb4000 commented May 25, 2022

template <class>
_INLINE_VAR constexpr bool test = false;

template <>
_INLINE_VAR constexpr bool test<char> = true;

in two different cpp files gives fatal error LNK1169: one or more multiply defined symbols found with clang-cl and C++14.

Fortunately, there is no code like this in the STL.

Ordinary constexpr variables or constexpr templates variables without explicit specialization work fine.

@CaseyCarter CaseyCarter added the bug Something isn't working label May 25, 2022
@CaseyCarter CaseyCarter added performance Must go faster and removed bug Something isn't working labels May 25, 2022
stl/inc/xlocale Outdated Show resolved Hide resolved
fsb4000 and others added 2 commits May 26, 2022 13:28
@fsb4000
Copy link
Contributor Author

fsb4000 commented May 26, 2022

Do we need remove_cv_t<_Ty> like there?:

STL/stl/inc/xtr1common

Lines 174 to 180 in 60decd0

template <class _Ty>
_INLINE_VAR constexpr bool is_integral_v = _Is_any_of_v<remove_cv_t<_Ty>, bool, char, signed char, unsigned char,
wchar_t,
#ifdef __cpp_char8_t
char8_t,
#endif // __cpp_char8_t
char16_t, char32_t, short, unsigned short, int, unsigned int, long, unsigned long, long long, unsigned long long>;

@StephanTLavavej
Copy link
Member

The type traits for the "primary type categories" like is_integral_v are required to ignore top-level cv-qualifiers (WG21-N4910 [meta.unary.cat]/2). Our internal type traits don't always remove_cv_t, but we need to keep this in mind whenever we're using them, as this is a potential source of bugs.

(_Is_any_of_v itself should not remove_cv_t as it is a generalized is_same which must care about cv-qualifiers; it's things like the "is it safe to activate the memmeow() optimization" helpers that sometimes expect usage to have already removed cv.)

strega-nil-ms
strega-nil-ms approved these changes May 31, 2022

template <class _Elem, class _Byte>
_INLINE_VAR constexpr bool _Is_codecvt_do_always_noconv_v =
is_same_v<_Byte, _Elem> || (_Is_one_byte_char_like_v<_Byte> && _Is_one_byte_char_like_v<_Elem>);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to also check for _Is_two_byte_char_like_v, for wchar_t/char16_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is an issue for that: #605 (comment)
but I have not yet explored what needs to be done there to speed up

@StephanTLavavej StephanTLavavej self-assigned this Jun 1, 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 a3c1eb2 into microsoft:main Jun 2, 2022
@StephanTLavavej
Copy link
Member

Thanks for this major performance improvement! 🚀 🐇 🏎️

@fsb4000 fsb4000 deleted the fix2109 branch June 2, 2022 04:15
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
performance Must go faster
Projects
None yet
5 participants