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

forward: single argument, copy const #5348

Merged
merged 1 commit into from Jan 17, 2018
Merged

Conversation

radcapricorn
Copy link
Contributor

Per discussion, augment std.functional.forward to:

  • support single-argument use case
  • copy immovables (i.e. const/immutable)

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

This LGTM as it allows use cases that didn't work before (using forward with const arguments).
However,

  • as mentioned I would move the full-blown example at the end of the public examples to avoid overloading the user in the beginning
  • (a bit related) I heard talks that there are plans to extend compiler support to automatically move a by-value argument if it's used only once, but it might take months or years until we see this happen...

std/functional.d Outdated

///
@safe unittest
{
Copy link
Member

Choose a reason for hiding this comment

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

I guess it would be nicer for the reader to have the simple example first before he has to start diving into the full-blown one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@radcapricorn
Copy link
Contributor Author

Since it's been a month, I've found another use case internally, so I'm adding it now.

@wilzbach
Copy link
Member

Merge remote-tracking branch 'upstream/master' into forward

Ehm you could you rebase in the future? (I can fix this one if we get a second approval)
It's rather simple:

git rebase upstream/master

@radcapricorn
Copy link
Contributor Author

Sorry, my bad. Of course.

@wilzbach
Copy link
Member

(I can fix this one if we get a second approval)

So I rebased and squashed to one commit and maybe we can get a second pairs?
CC @JackStouffer @quickfur

@wilzbach
Copy link
Member

wilzbach commented Jul 2, 2017

wilzbach commented 22 days ago

So I rebased and squashed to one commit and maybe we can get a second pairs?
CC @JackStouffer @quickfur

Reviewer roulette: @ZombineDev @CyberShadow @schveiguy @burner

@wilzbach wilzbach requested a review from quickfur July 8, 2017 22:19
@wilzbach
Copy link
Member

Reviewer roulette: @ZombineDev @CyberShadow @schveiguy @burner

Okay - I am going to merge this if there's no veto in the next seven days ;-)

Copy link
Member

@MetaLang MetaLang left a comment

Choose a reason for hiding this comment

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

LGTM

@property auto fwd(){ return move(arg); }

static if (args.length == 1)
alias forward = fwd;
Copy link
Member

Choose a reason for hiding this comment

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

Is this to avoid a template instantiation in the case where args.length == 1? If so, I wouldn't think it'd matter that much.

@dukc
Copy link
Contributor

dukc commented Aug 24, 2017

@wilzbach You forgot something here :-)

@PetarKirov
Copy link
Member

Let's first make sure this is not a breaking change, by waiting for a green Jenkins. The last error is unrelated, but before that this run https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fphobos/detail/PR-5348/3/pipeline looks like it might have failed because of this PR.

@schveiguy
Copy link
Member

@wilzbach What is the status of this?

@wilzbach wilzbach closed this Jan 16, 2018
@wilzbach wilzbach reopened this Jan 16, 2018
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @radcapricorn! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

@wilzbach
Copy link
Member

I think this just stalled due to spurious Jenkins failures. Let's see whether this still is the case...

@wilzbach
Copy link
Member

Manually merging as this PR seems to have exhausted its status updates:

image

@wilzbach wilzbach merged commit c8d8fe0 into dlang:master Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants