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

[span] Make span standard_layout #877

Merged
merged 10 commits into from
Jun 24, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jun 3, 2020

This moves span from inheritance to composition, which is problematic given that we define members both in the base class (_Mysize) and the derived class (_Mydata)

However, this is blocked on [[no_unique_address]] working on MSVC

@miscco miscco requested a review from a team as a code owner June 3, 2020 10:32
@AdamBucior
Copy link
Contributor

The design @ned14 proposed in #874 (comment) would be much better.

@miscco
Copy link
Contributor Author

miscco commented Jun 3, 2020

@AdamBucior couldyou elaborate on that as this is essentially the same design.

The one difference with inheritance would be that One would have to sprinkle this-> all over the place

@AdamBucior
Copy link
Contributor

The one difference with inheritance would be that One would have to sprinkle this-> all over the place

You can use using _Mybase::_Some_member

@miscco
Copy link
Contributor Author

miscco commented Jun 3, 2020

Again I am unsure what benefit inheritance has over composition?

@AdamBucior
Copy link
Contributor

AdamBucior commented Jun 3, 2020

Again I am unsure what benefit inheritance has over composition?

I feel like inheritance is more trustworthy and harder to get wrong. Inheritance is already widely used in the STL and span already uses it, so will be easier to merge. Also won't have to be blocked on no_unique_address.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jun 3, 2020
@miscco miscco force-pushed the span_standard_layout branch from bb77cec to 51f0d5b Compare June 3, 2020 13:14
@miscco
Copy link
Contributor Author

miscco commented Jun 3, 2020

Ok so given that MSVC does not support [[no_unique_address]] yet I went again with inheritance as this does not create any ABI problems.

@AlexGuteniev
Copy link
Contributor

this is blocked on [[no_unique_address]] working on MSVC

@aymarino explained:

Regarding the feature itself, we do plan on implementing it, but since it is an ABI-breaking change we will not be able to enable it by default until we release an ABI-breaking toolset.

See here: https://developercommunity.visualstudio.com/solutions/973748/view.html

So I'm afraid that if you can't do it without [[no_unique_address]], you cannot do it until vNext.

stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
tests/std/tests/P0122R7_span/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0122R7_span/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0122R7_span/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter linked an issue Jun 3, 2020 that may be closed by this pull request
@miscco miscco force-pushed the span_standard_layout branch from 9feaf11 to a5e7b32 Compare June 3, 2020 15:58
stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jun 3, 2020

Note that the _Other.size() calls in the constructors cannot be removed as _Other might be a different type where we do not have access to the private members

stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Show resolved Hide resolved
@miscco miscco force-pushed the span_standard_layout branch from 4743584 to 8b9843a Compare June 4, 2020 09:57
@miscco
Copy link
Contributor Author

miscco commented Jun 4, 2020

Ok. I am going out on a limb an declare that the futures::wait_until test was not broken by these changes.

@cbezault
Copy link
Contributor

cbezault commented Jun 4, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cbezault
Copy link
Contributor

cbezault commented Jun 4, 2020

We'll re-run and see if it's spurious. If it is I'll open an issue to track it/investigate it later.

stl/inc/span Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

I think this is a good improvement (despite requesting more information in #874 about why pointer-length layout is so important). Noticed minor stylistic issues.

stl/inc/span Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
stl/inc/span Outdated Show resolved Hide resolved
tests/std/tests/P0122R7_span/test.cpp Outdated Show resolved Hide resolved
stl/inc/span Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd like to push a change to reverse the internal constructor parameters.

stl/inc/span Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit 569efab into microsoft:master Jun 24, 2020
@StephanTLavavej
Copy link
Member

Thanks for simplifying span's representation and improving its debug codegen before the ABI freeze! 😺

ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jul 1, 2020
Fixes microsoft#874.

Co-authored-by: Casey Carter <[email protected]>
Co-authored-by: Stephan T. Lavavej <[email protected]>
@miscco miscco deleted the span_standard_layout branch July 7, 2020 21:29
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.

<span> does not have standard layout
8 participants