Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Jan 28, 2020

Description

This addresses #59 by adding the missing deleted overloads to basic_ostream<meow>::operator<<(bark)

The other changesin that proposal have already been implemented. As there was no switch the changes have been implemented unconditionally.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@miscco miscco requested a review from a team as a code owner January 28, 2020 20:59
@CaseyCarter CaseyCarter added the decision needed We need to choose something before working on this label Jan 28, 2020
@@ -912,15 +912,46 @@ basic_ostream<char, _Traits>& operator<<(
}

#ifdef __cpp_char8_t // These deleted overloads are specified in P1423.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to other reviewers: This means the char8_t overloads continue to be deleted even in sub-C++20 mode (e.g., if compiling with e.g. /std:c++17 /Zc:char8_t). I'm fine with this inconsistent behavior of char8_t vs. char16_t/char32_t, shout if you are not.

Copy link

Choose a reason for hiding this comment

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

That's fine, in any case only "he who likes pain" will use char8_t in C++20... Now, where is that Jens Gustedt pdf on modern C ... sorry talking to myself, have to go now... Good work Casey, thanks a lot, see you ...

@CaseyCarter CaseyCarter removed the decision needed We need to choose something before working on this label Jan 29, 2020
miscco and others added 2 commits January 30, 2020 09:39
Co-Authored-By: Casey Carter <cartec69@gmail.com>
@CaseyCarter CaseyCarter self-assigned this Jan 30, 2020
@miscco
Copy link
Contributor Author

miscco commented Jan 30, 2020

Thanks a lot @CaseyCarter.
Can't wait until the tests are ported so that you dont have to do all the firefighting.

@CaseyCarter
Copy link
Contributor

Can't wait until the tests are ported so that you dont have to do all the firefighting.

Heh - I grabbed this since (a) I implemented the original char8_t support in the STL, and (b) it seemed small enough that I could quickly throw together a test to validate that the now ill-formed stream insertion expressions are in fact ill-formed. Then I ran tests, and discovered this beauty:

void
print_log_value<wchar_t const*>::operator()( std::ostream& ostr, wchar_t const* t )
{
    ostr << ( t ? t : L"null string" );
}

in the Boost unit test library (reported at boostorg/test#249).

@CaseyCarter CaseyCarter changed the title Implement char8_t compatability remedies Implement char8_t compatibility remedies Jan 31, 2020
@miscco
Copy link
Contributor Author

miscco commented Jan 31, 2020

@CaseyCarter Thanks for the fix, although I would suggest to change it in a way compatible with #380 If it is ok for you I would rebase it on #380 and use the convention established there:

#ifndef _HAS_STREAM_INSERTIONS_REMOVED_BY_P1423
#define _HAS_STREAM_INSERTIONS_REMOVED_BY_P1423 (_HAS_FEATURES_REMOVED_IN_CXX20)
#endif // _HAS_STREAM_INSERTIONS_REMOVED_BY_P1423

and then guard those with

#if _HAS_STREAM_INSERTIONS_REMOVED_BY_P1423

Also is the paper really needed in the name?

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jan 31, 2020

@CaseyCarter Thanks for the fix, although I would suggest to change it in a way compatible with #380 ... Also is the paper really needed in the name?

We didn't give the macro name a ton of thought beyond the fact that only I thought BOGUS_STREAM_INSERTIONS was a good name. In context with #380, I believe _HAS_STREAM_INSERTIONS_REMOVED_IN_CXX20 works quite well. I'll change the macro name and default to (!_HAS_CXX20), and work with @StephanTLavavej on the details of defaulting to _HAS_FEATURES_REMOVED_IN_CXX20. (I think this will be ready to merge in a few hours, but haven't seen Stephan's PR for #380 yet, so we may do the integration in #380 rather than here).

Does that sound good?

@miscco
Copy link
Contributor Author

miscco commented Feb 1, 2020

I am totally fine with the name.

@CaseyCarter CaseyCarter merged commit ed70349 into microsoft:master Feb 1, 2020
@CaseyCarter
Copy link
Contributor

Thanks for your contribution!

@miscco miscco deleted the char8_t branch February 2, 2020 20:23
@CaseyCarter CaseyCarter removed their assignment Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants