Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Jul 2, 2020

Note this is already written forward looking to what comes with the other PRs namely #959 and #930, so it will only compile after they have been merged. That said I plan to squeeze one of those into my lunch break and rebasing them should be faster than writing them some time in the future.

@miscco miscco requested a review from a team as a code owner July 2, 2020 12:10
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jul 2, 2020
@StephanTLavavej

This comment has been minimized.

@miscco
Copy link
Contributor Author

miscco commented Jul 3, 2020

So this is really strange. For whatever reason it tries to copy the int_wrapper class rather than moving it.

Any lead on why?

@CaseyCarter
Copy link
Contributor

So this is really strange. For whatever reason it tries to copy the int_wrapper class rather than moving it.

Any lead on why?

image

@miscco
Copy link
Contributor Author

miscco commented Jul 5, 2020

So it seems that the test machinery requires the elements to be copy constructible and copy assignable.

Is that intentional?

@miscco

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej
Copy link
Member

If you want to trigger a test run, you can push commits changing and undoing some comment; you might also be able to intentionally form an empty commit and push that (I know those can be created and pushed; not sure if Azure Pipelines will trigger based on them).

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 6, 2020

So it seems that the test machinery requires the elements to be copy constructible and copy assignable.

Is that intentional?

Nope: it's a bug in test::proxy_reference. We need test::proxy_reference<Cat, Elem> and T to model std::common_reference_with whenever Elem& and T do. Fixing it causes many of the tests to fail in MSVC's permissive mode, so they'll effectively need to be constrained to only test ranges with non-proxy-reference-types when compiling in permissive mode.

Biggish patch incoming, which I assume will conflict with many of the "Modernize meow" PRs.

EDIT: I've managed to greatly limit the impact of the changes after reverting a change that made proxy_reference's explicit constructor implicit. I made that change investigating a different formulation that tried to make proxy_reference itself the common_reference of proxy_reference and T before I landed on creating a new type. This may not be as bad/impactful/destabilizing to the ongoing work as I feared.

`test::proxy_reference<Cat, Elem>` and an arbitrary type `T` must model `common_reference_with` when `Elem&` and `T` do.

Drive-by:
* Change `proxy_reference<Cat, Elem>`'s conversion operator to `remove_cv_t<Elem>` into a conversion to `Elem&`, which is both simpler and more generic.
* Generalize `proxy_reference<Cat, Elem>`'s assignment from `remove_cv_t<Elem> const&` into a perfect-forwarding assignment operator.
* Extract `ranges::equal` out of `instantiator::call` into a separate function to keep `/analyze` from blowing the compiler's available heap space.
miscco and others added 2 commits July 7, 2020 16:26
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@miscco
Copy link
Contributor Author

miscco commented Jul 7, 2020

@CaseyCarter thanks a lot for the fix, this was way over my head. Am I correct that this also solves the problems with the meow_copy algorithms that seem to fail for strange reasons?

I will remove the copy costructor/assignment from int_wrapper to be sure

@CaseyCarter
Copy link
Contributor

@CaseyCarter thanks a lot for the fix, this was way over my head. Am I correct that this also solves the problems with the meow_copy algorithms that seem to fail for strange reasons?

I don't know, but I hope so ;)

I will remove the copy constructor/assignment from int_wrapper to be sure

I did so in this PR already, IIRC.

FWIW, there are still a couple (or I should say were a couple when the test runs weren't all failing spuriously) of test failures that repro on Azure but not on my local machine - possibly some issue that was fixed in 16.7p3. I'm not sure if we should try to localize and workaround or just update the test bots to the newer Preview since they're not exactly healthy right now.

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jul 8, 2020
@CaseyCarter

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej self-assigned this Jul 10, 2020
@StephanTLavavej StephanTLavavej merged commit 1f0f88f into microsoft:master Jul 11, 2020
@StephanTLavavej
Copy link
Member

Seeing all these cleanups truly moves me to tears. 😹

@miscco miscco deleted the ranges_move branch July 12, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants