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.

Sign up for GitHub

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

Jump to bottom

<functional>: std::function doesn't handle over-aligned types #698

Merged
merged 28 commits into from
Apr 22, 2020
Merged

<functional>: std::function doesn't handle over-aligned types #698

merged 28 commits into from
Apr 22, 2020

Conversation

AlexGuteniev
Copy link
Contributor

#690 , starting with tests to make sure they fail

@AlexGuteniev AlexGuteniev mentioned this pull request Apr 9, 2020
Closed
@AlexGuteniev AlexGuteniev marked this pull request as ready for review April 9, 2020 05:26
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner April 9, 2020 05:26
@miscco
Copy link
Contributor

miscco commented Apr 9, 2020 •
edited
Loading

Could you please move development offline?

I try to follow the development of the STL and if you push every single commit it really spams my inbox. This PR alone was 6 mails

It would be great if you could finish a task and then push that for review not every step in between.

@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Apr 9, 2020 •
edited
Loading

It would be great if you could finish a task and then push that for review not every step in between.

I'll try, but it is not always possible.

I did make this change locally working, including local test run. Test failure on CI was unexpected, but that's what CI is about - to run more tests under more conditions than I would do locally.

Also I deliberately added tests before the fix to make sure that if tests pass, they really pass, not ignored. This caused extra commit.

@CaseyCarter CaseyCarter added the bug Something isn't working label Apr 9, 2020
@CaseyCarter CaseyCarter linked an issue Apr 9, 2020 that may be closed by this pull request
<functional>: std::function doesn't handle over-aligned types #690
Closed
CaseyCarter
CaseyCarter requested changes Apr 9, 2020
View reviewed changes
stl/inc/functional Outdated Show resolved Hide resolved
tests/std/test.lst Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/VSO_1062649_overaligned_function/test.cpp Outdated Show resolved Hide resolved
CaseyCarter
CaseyCarter requested changes Apr 9, 2020
View reviewed changes
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

One last static_assert issue here and this looks good to me.

tests/std/tests/GH_000690_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_000690_overaligned_function/test.cpp Outdated Show resolved Hide resolved
CaseyCarter
CaseyCarter reviewed Apr 9, 2020
View reviewed changes
stl/inc/functional Outdated Show resolved Hide resolved
StephanTLavavej
StephanTLavavej requested changes Apr 9, 2020
View reviewed changes
stl/inc/functional Outdated Show resolved Hide resolved
stl/inc/functional Outdated Show resolved Hide resolved
tests/std/tests/GH_000690_overaligned_function/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_000690_overaligned_function/test.cpp Outdated Show resolved Hide resolved
BillyONeal
BillyONeal reviewed Apr 10, 2020
View reviewed changes
stl/inc/atomic Outdated Show resolved Hide resolved
@AlexGuteniev
Revert "Removing macros, now I have ICE"
01d8f43 
This reverts commit 76bfccb.
It was meant for another branch
@StephanTLavavej StephanTLavavej self-assigned this Apr 21, 2020
StephanTLavavej
StephanTLavavej approved these changes Apr 21, 2020
View reviewed changes
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! I pushed very small edits to the comments. Now I'll mirror this to the Microsoft-internal repo and merge this soon.

@StephanTLavavej StephanTLavavej merged commit 97f9a2c into microsoft:master Apr 22, 2020
@StephanTLavavej
Copy link
Member

Thanks for this significant bugfix! 🎉

@AlexGuteniev AlexGuteniev deleted the function_alinged branch April 23, 2020 14:40
@CaseyCarter CaseyCarter mentioned this pull request May 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<functional>: std::function doesn't handle over-aligned types
5 participants
@AlexGuteniev @miscco @StephanTLavavej @CaseyCarter @BillyONeal