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.

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

use constructors instead of move #6939

Merged
merged 1 commit into from
Apr 1, 2019
Merged

Conversation

aG0aep6G
Copy link
Contributor

This is a fix-up of #6346 and #6935.

As far as I understand, @WalterBright favors this style (cf. #6900, #6903).

Most of these are straight forward, but chain and roundRobin are still a bit awkward, because they involve tuples. We don't have some kind of map for tuples in Phobos, do we?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @aG0aep6G! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + phobos#6939"

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Craftily done. One possibility would be to (re)do this when copy constructors get released (they just got merged). Anyhow, worksforme. @WalterBright?

@aG0aep6G
Copy link
Contributor Author

One possibility would be to (re)do this when copy constructors get released

I'm not sure how you'd use copy constructors here. Make every copy save the range? That's fine for new code, but changing the copy behavior of existing types seems like asking for trouble.

@aG0aep6G
Copy link
Contributor Author

I saw CircleCI and Buildkite failing with errors that seemed unrelated, so I thought I'd give them a kick with a force push. But I guess that was a bad idea. It only managed to remove the auto-merge label. Checks are still failing.

Can someone help me out here?

@thewilsonator
Copy link
Contributor

If you haven't rebased this recently please do so.

@thewilsonator
Copy link
Contributor

Yep, this was forked off from an old master we've since fixed the issues with circle but you need to have branched off after that to see those fixes on this PR, a rebase will fix your problems.

@aG0aep6G
Copy link
Contributor Author

Yep, this was forked off from an old master we've since fixed the issues with circle but you need to have branched off after that to see those fixes on this PR, a rebase will fix your problems.

I've rebased, but this PR is against stable, not master. Does that make a difference?

@thewilsonator
Copy link
Contributor

Ah, circle will still fail, but is not require to pass, sorry I missed that. I'm more concerned about the build kite failure but that might just be an unfortunate feature of the current stable.

@thewilsonator
Copy link
Contributor

In short this needs a manual merge, which I will do tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants