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

<chrono>: [time.duration.io] output should handle types beyond char and wchar_t #1430

Closed
StephanTLavavej opened this issue Nov 6, 2020 · 4 comments
Labels
bug Something isn't working invalid This issue is incorrect or by design

Comments

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Nov 6, 2020

When #1341 implemented operator<<(basic_ostream&, const duration&), its unit suffix logic handled only char and wchar_t:

STL/stl/inc/chrono

Lines 625 to 662 in 6458199

#define _IF_PERIOD_RETURN_SUFFIX_ELSE(_TYPE, _SUFFIX) \
if constexpr (is_same_v<_Period, _TYPE>) { \
if constexpr (is_same_v<_CharT, char>) { \
return _SUFFIX; \
} else { \
return L##_SUFFIX; \
} \
} else
template <class _CharT, class _Period>
_NODISCARD constexpr const _CharT* _Get_literal_unit_suffix() {
_IF_PERIOD_RETURN_SUFFIX_ELSE(atto, "as")
_IF_PERIOD_RETURN_SUFFIX_ELSE(femto, "fs")
_IF_PERIOD_RETURN_SUFFIX_ELSE(pico, "ps")
_IF_PERIOD_RETURN_SUFFIX_ELSE(nano, "ns")
_IF_PERIOD_RETURN_SUFFIX_ELSE(micro, "us")
_IF_PERIOD_RETURN_SUFFIX_ELSE(milli, "ms")
_IF_PERIOD_RETURN_SUFFIX_ELSE(centi, "cs")
_IF_PERIOD_RETURN_SUFFIX_ELSE(deci, "ds")
_IF_PERIOD_RETURN_SUFFIX_ELSE(seconds::period, "s")
_IF_PERIOD_RETURN_SUFFIX_ELSE(deca, "das")
_IF_PERIOD_RETURN_SUFFIX_ELSE(hecto, "hs")
_IF_PERIOD_RETURN_SUFFIX_ELSE(kilo, "ks")
_IF_PERIOD_RETURN_SUFFIX_ELSE(mega, "Ms")
_IF_PERIOD_RETURN_SUFFIX_ELSE(giga, "Gs")
_IF_PERIOD_RETURN_SUFFIX_ELSE(tera, "Ts")
_IF_PERIOD_RETURN_SUFFIX_ELSE(peta, "Ps")
_IF_PERIOD_RETURN_SUFFIX_ELSE(exa, "Es")
_IF_PERIOD_RETURN_SUFFIX_ELSE(minutes::period, "min")
_IF_PERIOD_RETURN_SUFFIX_ELSE(hours::period, "h")
_IF_PERIOD_RETURN_SUFFIX_ELSE(ratio<86400>, "d")
{
return nullptr;
}
}
#undef _IF_PERIOD_RETURN_SUFFIX_ELSE

However, WG21-N4868 27.5.11 [time.duration.io]/1 doesn't limit charT to being only char and wchar_t. Instead, it depicts narrow string literals being directly streamed into the basic_ostream regardless of its charT (which works because const char * is always streamable, albeit with reduced performance if the types differ).

Retaining the wchar_t logic is permitted and improves performance, so we just need to fix this for other character types (like char8_t, char16_t, and char32_t).

@MattStephanson's first suggested strategy in #1341 (comment) sounds good:

I think I can just switch the order of the if/else to make char the fallback, then use a conditional to get the correct return type.

We should also add test coverage for this - it doesn't have to be exhaustive, just testing a literal and a general suffix for another character type would be sufficient.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Nov 6, 2020
@MattStephanson
Copy link
Contributor

Are they any tricks for using iostreams classes with char??_t? I tried to test this with a basic_stringstream<char16_t>, and got an error from

PM_CL="/FIforce_include.hpp /w14365 /D_ENFORCE_FACET_SPECIALIZATIONS=1"

static_assert(!_ENFORCE_FACET_SPECIALIZATIONS || _Always_false<_Elem>, _FACET_SPECIALIZATION_MESSAGE);

_FACET_SPECIALIZATION_MESSAGE refers to [locale.category], though perhaps [iostreams.limits.pos]/2 is relevant too. The only instantiations for num_put are for char, wchar_t, and unsigned short (fallback for /Zc:wchar_t-?), so it doesn't look like the library supports anything else, which would make this issue moot.

STL/stl/inc/xlocnum

Lines 1619 to 1635 in 19c683d

#if !defined(_CRTBLD) || defined(__FORCE_INSTANCE)
template __PURE_APPDOMAIN_GLOBAL locale::id numpunct<char>::id;
template class _CRTIMP2_PURE_IMPORT num_get<char, istreambuf_iterator<char, char_traits<char>>>;
template class _CRTIMP2_PURE_IMPORT num_put<char, ostreambuf_iterator<char, char_traits<char>>>;
template __PURE_APPDOMAIN_GLOBAL locale::id numpunct<wchar_t>::id;
template class _CRTIMP2_PURE_IMPORT num_get<wchar_t, istreambuf_iterator<wchar_t, char_traits<wchar_t>>>;
template class _CRTIMP2_PURE_IMPORT num_put<wchar_t, ostreambuf_iterator<wchar_t, char_traits<wchar_t>>>;
#endif // !defined(_CRTBLD) || defined(__FORCE_INSTANCE)
#ifdef __FORCE_INSTANCE
template __PURE_APPDOMAIN_GLOBAL locale::id numpunct<unsigned short>::id;
template class _CRTIMP2_PURE_IMPORT
num_get<unsigned short, istreambuf_iterator<unsigned short, char_traits<unsigned short>>>;
template class _CRTIMP2_PURE_IMPORT
num_put<unsigned short, ostreambuf_iterator<unsigned short, char_traits<unsigned short>>>;
#endif // __FORCE_INSTANCE

@CaseyCarter
Copy link
Member

CaseyCarter commented Nov 16, 2020

Are they any tricks for using iostreams classes with char??_t?

The iostreams class templates only support characters of type char and wchar_t (and unsigned short when /Zc:wchar_t-, but don't do that). The fact that many of the templates can be instantiated with other types is an unfortunate historical artifact of our implementation.

That said, I'm not sure what @StephanTLavavej has in mind for support of other character types.

@MattStephanson
Copy link
Contributor

@StephanTLavavej, what do you think of this, in light of @CaseyCarter's comments? It looks like the library is not required, and does not attempt, to support iostream classes using other than char and wchar_t.

@StephanTLavavej
Copy link
Member Author

Yep, I agree that this is a non-bug. Thanks for looking into it (and apologies for the delayed response).

@StephanTLavavej StephanTLavavej added the invalid This issue is incorrect or by design label Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This issue is incorrect or by design
Projects
None yet
Development

No branches or pull requests

3 participants