Remove unnecessary explicit constructors #415

miscco · 2020-01-09T20:39:33Z

Description

This fixes #41 by unconditionally defining the default constructors

In contrast to the paper the constructors do not utilize the explicit constructors as that seems to be against the common praxis of the STL

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.

BillyONeal · 2020-01-09T20:54:44Z

stl/inc/strstream

@@ -34,7 +34,12 @@ public:
    }; // set if character array ownership given away
    using _Strstate = int;

-    explicit __CLR_OR_THIS_CALL strstreambuf(streamsize _Count = 0) {
+    // construct with empty character array
+    __CLR_OR_THIS_CALL strstreambuf() {


This looks like it might be an ABI break (otherwise why would it be annotated __CLR_OR_THIS_CALL ?)

Alternately the __CLR_OR_THIS_CALL annotation might be incorrect.

It is definitely concerning - the presence of explicit calling conventions strongly indicates separately compiled machinery that we can't mess with. However, in the specific case of strstreambuf, I believe that this may be okay. The base class basic_streambuf has explicit calling conventions because it is indeed separately compiled. However, I can't find any mentions of strstream (including strstreambuf) in either the static or dynamic libraries. Therefore, I believe that splitting this constructor should be ABI-safe. Our test to verify the export surface will serve as a double-check.

I agree with Stephan's analysis. If none of these are exported, should we file a separate issue to remove __CLR_OR_THIS_CALL from all of this class's function declarations?

I think that would be unnecessarily risky for no benefit. This is strstream, after all - the entire thing is deprecated and rarely changes. Also, the virtuals need explicitly specified calling conventions to match the base class, even if the non-virtuals don't.

(Also, vNext will eliminate separately compiled classes.)

NO chage done

BillyONeal · 2020-01-09T20:55:42Z

In contrast to the paper the constructors do not utilize the explicit constructors as that seems to be against the common praxis of the STL

Yes, the codegen for that is very poor due to the interaction with EH :(

stl/inc/cvt/wbuffer

CaseyCarter · 2020-01-09T22:29:13Z

stl/inc/strstream

@@ -34,7 +34,12 @@ public:
    }; // set if character array ownership given away
    using _Strstate = int;

-    explicit __CLR_OR_THIS_CALL strstreambuf(streamsize _Count = 0) {
+    // construct with empty character array
+    __CLR_OR_THIS_CALL strstreambuf() {


I agree with Stephan's analysis. If none of these are exported, should we file a separate issue to remove __CLR_OR_THIS_CALL from all of this class's function declarations?

stl/inc/strstream

CaseyCarter

Pushed a commit with some comment-wrapping cleanup.

timsong-cpp

The distributions' param_type are specified by reference to the distributions' constructors, so the changes to the distributions need to apply to their param_type as well. (This is intentional.)

miscco · 2020-01-15T15:41:21Z

@timsong-cpp Thanks a lot for the hint. I was unaware of that connection. Fixed it and pushed

StephanTLavavej

Looks great. I found a couple of extremely minor comment/whitespace nitpicks, but I believe this is ready to merge as-is. (I double-checked the paper and verified that all edits were applied or already present.) We might want to add tests for this (although I am tempted to be lazy).

stl/inc/sstream

stl/inc/xlocbuf

StephanTLavavej · 2020-01-24T07:37:22Z

Rebased to resolve merge conflict in yvals_core.h. This has passed my test run; beginning the final Microsoft-internal PR now.

StephanTLavavej · 2020-01-24T20:01:42Z

Thanks for implementing another C++20 feature! This will appear in VS 2019 16.6 Preview 1.

miscco requested a review from a team as a code owner January 9, 2020 20:39

miscco force-pushed the non_explicit branch from 2ae22b4 to 3da4364 Compare January 9, 2020 20:43

BillyONeal approved these changes Jan 9, 2020

View reviewed changes

CaseyCarter requested changes Jan 9, 2020

View reviewed changes

miscco force-pushed the non_explicit branch 2 times, most recently from 402292e to 1c38b3f Compare January 10, 2020 04:32

CaseyCarter approved these changes Jan 10, 2020

View reviewed changes

timsong-cpp suggested changes Jan 15, 2020

View reviewed changes

miscco force-pushed the non_explicit branch from eb1aaba to 3dbb006 Compare January 15, 2020 15:40

miscco force-pushed the non_explicit branch from 3dbb006 to 3602a4d Compare January 15, 2020 19:31

StephanTLavavej approved these changes Jan 22, 2020

View reviewed changes

stl/inc/sstream Outdated Show resolved Hide resolved

stl/inc/xlocbuf Outdated Show resolved Hide resolved

StephanTLavavej self-assigned this Jan 22, 2020

miscco force-pushed the non_explicit branch from 3602a4d to cbde18f Compare January 22, 2020 05:41

StephanTLavavej approved these changes Jan 22, 2020

View reviewed changes

chamajcpradel approved these changes Jan 24, 2020

View reviewed changes

miscco and others added 4 commits January 23, 2020 23:35

[random] Make most distributions default constructible

d00d30b

[streams] Make more streams default constructible

0abc342

[wbuffer] Make wbuffer_convert default constructible

b2325a9

Wrap some comments

cb20871

StephanTLavavej force-pushed the non_explicit branch from cbde18f to cb20871 Compare January 24, 2020 07:36

StephanTLavavej approved these changes Jan 24, 2020

View reviewed changes

StephanTLavavej merged commit 0e336ac into microsoft:master Jan 24, 2020

miscco deleted the non_explicit branch January 28, 2020 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary explicit constructors #415

Remove unnecessary explicit constructors #415

miscco commented Jan 9, 2020

BillyONeal Jan 9, 2020

StephanTLavavej Jan 9, 2020

CaseyCarter Jan 9, 2020 •

edited

Loading

StephanTLavavej Jan 9, 2020

miscco Jan 10, 2020

BillyONeal commented Jan 9, 2020

CaseyCarter Jan 9, 2020 •

edited

Loading

CaseyCarter left a comment

timsong-cpp left a comment •

edited

Loading

miscco commented Jan 15, 2020

StephanTLavavej left a comment

StephanTLavavej commented Jan 24, 2020

StephanTLavavej commented Jan 24, 2020

Remove unnecessary explicit constructors #415

Remove unnecessary explicit constructors #415

Conversation

miscco commented Jan 9, 2020

Description

Checklist

BillyONeal Jan 9, 2020

Choose a reason for hiding this comment

StephanTLavavej Jan 9, 2020

Choose a reason for hiding this comment

CaseyCarter Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

StephanTLavavej Jan 9, 2020

Choose a reason for hiding this comment

miscco Jan 10, 2020

Choose a reason for hiding this comment

BillyONeal commented Jan 9, 2020

CaseyCarter Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

CaseyCarter left a comment

Choose a reason for hiding this comment

timsong-cpp left a comment • edited Loading

Choose a reason for hiding this comment

miscco commented Jan 15, 2020

StephanTLavavej left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Jan 24, 2020

StephanTLavavej commented Jan 24, 2020

CaseyCarter Jan 9, 2020 •

edited

Loading

CaseyCarter Jan 9, 2020 •

edited

Loading

timsong-cpp left a comment •

edited

Loading