Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Jul 12, 2020

This implements ranges::unique and ranges::unique_copy.

@CaseyCarter: Any good idea on how to test which implementation is actually used? I thought about counting inside the projection if the projected object lies within input or output or somewhere else. But that does not fully work because some of the combinations of the store path trigger the higher ones.

@miscco miscco requested a review from a team as a code owner July 12, 2020 10:13
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Jul 12, 2020
@CaseyCarter CaseyCarter mentioned this pull request Jul 12, 2020
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jul 12, 2020
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@miscco
Copy link
Contributor Author

miscco commented Jul 13, 2020

For whatever reasons @github does not update the branch. Seems it has trouble

@StephanTLavavej
Copy link
Member

GitHub apparently had some downtime earlier, but I see a code format validation failure because my requested comment edit made the line exceed 120 characters 😹 I think if you fix that, it should pass?

@miscco
Copy link
Contributor Author

miscco commented Jul 13, 2020

I already fixed that and pushed it to github so I do not really know what to do that it picks up the new commit

miscco@89b50da

@CaseyCarter
Copy link
Contributor

@CaseyCarter: Any good idea on how to test which implementation is actually used? I thought about counting inside the projection if the projected object lies within input or output or somewhere else. But that does not fully work because some of the combinations of the store path trigger the higher ones.

I recommend embracing that fact that each of the generator templates is going to generate cases that are handled by each of the three backends in the algorithm. Analyze instantiator::call's template parameters to determine which backend will be used, and then verify that it is.

Copy link
Contributor

@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.

LGTM after addressing Stephan's comments.

miscco and others added 2 commits July 14, 2020 09:27
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@miscco
Copy link
Contributor Author

miscco commented Jul 15, 2020

hiss at clang

I guess we should limit some of the instantiations here

@miscco
Copy link
Contributor Author

miscco commented Jul 18, 2020

So ranges::unique_copy should be ready to go.

ranges::unique: I got an ICE for you...

@miscco
Copy link
Contributor Author

miscco commented Jul 18, 2020

As far as I can tell the new unwrapping for continuous ranges leads to the ICE.

If i go back to b4fb1f9 and add that lines and the changes to _Seek_to I get the ICE

@miscco
Copy link
Contributor Author

miscco commented Jul 21, 2020

ssiH at reverse

@CaseyCarter
Copy link
Contributor

As far as I can tell the new unwrapping for continuous ranges leads to the ICE.

Yes; I applied the if constexpr (!ranges::contiguous_range<ReadWrite>) workaround for VSO-938163 after observing the "all MSVC configs ICE" syndrome. In the unique_copy test, I simplified the projection counting by using the non-key pair member to store the count and split the iterator overload / range overload into separate instantions so TEST_EVERYTHING actually compiles.

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 modulo two trivial nitpicks.

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@CaseyCarter CaseyCarter removed their assignment Jul 22, 2020
@CaseyCarter CaseyCarter self-assigned this Jul 27, 2020
@CaseyCarter CaseyCarter merged commit 28efc70 into microsoft:master Jul 27, 2020
@CaseyCarter
Copy link
Contributor

Thanks for your, err, one-of-a-kind contribution.

@CaseyCarter CaseyCarter removed their assignment Jul 27, 2020
@miscco miscco deleted the ranges_unique branch July 27, 2020 18:48
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Jul 28, 2020
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Co-authored-by: Casey Carter <Casey@Carter.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants