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

Remove _ITERATOR_DEBUG_ARRAY_OVERLOADS #735

Merged
merged 5 commits into from
Apr 25, 2020
Merged

Remove _ITERATOR_DEBUG_ARRAY_OVERLOADS #735

merged 5 commits into from
Apr 25, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Apr 21, 2020

This fixes #660

I ran the tests that were visibly affected on my machine. That said I am curious about the tests on other architectures

@miscco miscco requested a review from a team as a code owner April 21, 2020 20:24
BillyONeal
BillyONeal reviewed Apr 21, 2020
View reviewed changes
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
BillyONeal
BillyONeal reviewed Apr 21, 2020
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Can _Array_iterator or other machinery in <xutility> be pushed down now?

tests/std/include/instantiate_algorithms.hpp Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Apr 21, 2020

Can _Array_iterator or other machinery in <xutility> be pushed down now?

I am unsure whether I am the best to answer that but will have a look tomorrow

@BillyONeal
Copy link
Member

Can _Array_iterator or other machinery in <xutility> be pushed down now?

I am unsure whether I am the best to answer that but will have a look tomorrow

I just did a grep and it looks like that thing is only used by <array> after this change so it can be moved there, but I might have missed something.

Thanks for your contribution!

@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Apr 22, 2020
@miscco
Copy link
Contributor Author

miscco commented Apr 22, 2020

I moved _Array_const_iterator and _Array_iterator to <array>

@StephanTLavavej
_HAS_CXX17 implies _HAS_IF_CONSTEXPR.
ef97636 
This is an unrelated cleanup.
StephanTLavavej
StephanTLavavej approved these changes Apr 23, 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 great - thanks for the extensive test updates, moving the array iterators, and removing the now-unused _STL_VERIFY_ARRAY_SIZE. You really went above and beyond!

In fact, this change was so good, I noticed that the surrounding code in the STL was bogus in comparison. Specifically, near an IDAO removal, I noticed that we were testing _HAS_IF_CONSTEXPR within _HAS_CXX17 (which is totally unnecessary - C++17 implies if constexpr availability). I found no other occurrences throughout the STL, so I simply pushed a commit to your PR.

@CaseyCarter CaseyCarter self-assigned this Apr 23, 2020
@BillyONeal
Copy link
Member

Thanks so much :D

@CaseyCarter CaseyCarter merged commit ca032ae into microsoft:master Apr 25, 2020
@CaseyCarter
Copy link
Member

Thanks again, @miscco!

@miscco miscco deleted the array_overload branch June 3, 2020 13:31
@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
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Assignees
No one assigned
Labels
throughput Must compile faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<algorithm> / <numeric> / <xutility>: _ITERATOR_DEBUG_ARRAY_OVERLOADS should be removed
4 participants
@miscco @BillyONeal @CaseyCarter @StephanTLavavej