Skip to content

Conversation

@fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Sep 18, 2020

@fsb4000 fsb4000 requested a review from a team as a code owner September 18, 2020 20:59
@fsb4000 fsb4000 force-pushed the master branch 3 times, most recently from 32f607a to ad1a0aa Compare September 18, 2020 22:38
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Sep 18, 2020
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.

Thanks for implementing this feature (over 4% of the remaining features 😹)! I think it'll be ready to merge after one round of revisions.

@fsb4000 fsb4000 force-pushed the master branch 4 times, most recently from f1f2363 to c5a09db Compare September 19, 2020 02:44
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.

This looks good, thanks! I pushed a couple of changes to the product code:

  1. Because Clang works, we should use #ifndef __clang__ to guard the workaround, so it activates for MSVC and EDG only.
  2. We can "extract" the non-workaround code, so it doesn't have to be repeated.

I also changed the #endif comment, to make it super clear that the whole block is the workaround that should be removed. (Sometimes when a workaround is necessary for years, it's easy to forget what we should do when the relevant bug is finally fixed.)

I'll push one more change to the test code, then I think this will be ready for final review!

@StephanTLavavej
Copy link
Member

One more thing that I forgot to mention - squashing your development history down to a single commit before creating a PR (as you did here) is great. (Creating a PR with separate commits can make sense for mostly-unrelated changes or multi-layered rewrites; I do that occasionally.) However, after a PR has been created, further changes should generally be pushed without rewriting history. This is because GitHub's interface makes it relatively easy to see "commits added since my last review", but makes it very hard to see what happened between force-pushes.

Totally not a problem for this PR, just advice for larger PRs in the future. 😺

`assume_aligned` never modifies its parameter.
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.

Thanks! I pushed a small change to restore the comment explaining how the Standard's power-of-two requirement is being enforced.

@StephanTLavavej StephanTLavavej self-assigned this Sep 22, 2020
@StephanTLavavej StephanTLavavej merged commit 6e6321c into microsoft:master Sep 23, 2020
@StephanTLavavej
Copy link
Member

Thanks for aligning the STL's feature set with the C++20 Standard, and congratulations on implementing your first feature! 🎉 🚀 😺

@fsb4000 fsb4000 deleted the master branch June 13, 2021 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P1007R3 assume_aligned()

4 participants