Skip to content

Add std.functional.acceptRVals #3937

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

Closed
wants to merge 1 commit into from
Closed

Conversation

tsbockman
Copy link
Contributor

@TurkeyMan std.functional.acceptRVals is a mixin template that generates a function wrapper that will accept rvalue arguments, even for ref parameters.

I've attempted to properly handle all the weird stuff like return ref, ref return type, function attributes, class methods, variadic functions, etc.

The main thing it doesn't do, is automate wrapping a template function from the outside. You either have to wrap it from the inside, or wrap a specific instantiation.

TODO: I should add more unit tests, but I figure this is enough for me to request feedback.

@tsbockman tsbockman force-pushed the acceptrvals branch 2 times, most recently from fdaba47 to 85de341 Compare January 19, 2016 00:15
@JackStouffer
Copy link
Member

Nitpick: rvalue is one word no? So shouldn't this be acceptRvals?

@tsbockman
Copy link
Contributor Author

Nitpick: rvalue is one word no?

"rvalue" is not an English word; if it was there would be a vowel or two attached to the "r" giving some hint as to how to pronounce it. It's really an abbreviation for something like "right-hand value".

(Of course, I can still change the capitalization if that's what people really want.)

@tsbockman
Copy link
Contributor Author

I included a unittest block specifically for the docs, but it's not showing up in them when I build on my local machine.

Anyone know why? I thought all I had to do was stick /// on the line above.

assert(x == 0);

// The wrapped function can be anonymous:
mixin acceptRVals!("mul", function(ref const(int) a, ref const(int) b) {
Copy link
Member

Choose a reason for hiding this comment

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

For readability's sake, could you please break these up into multiple ddoc-ed unit tests.

@JackStouffer
Copy link
Member

"rvalue" is not an English word

Sure it is, it's just jargon.

@tsbockman
Copy link
Contributor Author

Sure it is, it's just jargon.

You asked for my opinion; I gave it. This is a silly thing to argue about.

If there is consensus that I should change it, I will do so.

@tsbockman tsbockman force-pushed the acceptrvals branch 2 times, most recently from fd07a67 to 18688f0 Compare January 19, 2016 02:11
@tsbockman
Copy link
Contributor Author

I included a unittest block specifically for the docs, but it's not showing up in them when I build on my local machine.

The unittest blocks are showing up now, but ddoc is still mis-labeling the mixin template as a regular template.

@tsbockman tsbockman force-pushed the acceptrvals branch 3 times, most recently from 67ce32a to 633488a Compare January 19, 2016 05:59
@andralex
Copy link
Member

This doesn't look well. Part of the problem is it's an all-or-nothing proposition for all parameters. Not to mention it's awkward to use.

A possibility would be to add this:

ref T ignoreLval(uint param, T)(T value)
{
    static T lvalue;
    move(value, lvalue);
    return lvalue;
}

Then people who want to call add(ref int, ref int) with rvalues write add(ignoreLval!1(42), ignoreLval!2(43)).

@tsbockman
Copy link
Contributor Author

@andralex

Part of the problem is it's an all-or-nothing proposition for all parameters.

That's kind of the point. People expect passing rvalues by reference to "just work" as long as the reference isn't being escaped.

I exempted return ref parameters since that's currently the only way of explicitly marking something as possibly escaped. If you want to do better, implement scope.

@tsbockman tsbockman closed this Jan 19, 2016
@tsbockman tsbockman reopened this Jan 19, 2016
@tsbockman
Copy link
Contributor Author

Oops. I accidentally hit close; ignore that.

@tsbockman
Copy link
Contributor Author

@andralex

ref T ignoreLval(uint param, T)(T value)

I really don't think anyone wants that; it has the same poor scaling as just making temporary variables: it requires additional code for every single function call with rvalues, rather than just modifying the declaration.

Also, it depends on TLS which could be substantially slower than my solution unless the optimizer is smarter than I think. One of the reasons that people (i.e. Manu) want to use ref everywhere is for the speed boost versus copying.

@tsbockman
Copy link
Contributor Author

@andralex

ref T ignoreLval(uint param, T)(T value)
{
    static T lvalue;
    move(value, lvalue);
    return lvalue;
}

That struck me as unsafe from the moment I saw it, and I finally figured out why: in a (directly or indirectly) recursive function, lvalue could get overwritten by an inner call before the outer call(s) are done with it. It would be equivalent to inadvertently overwriting part of the caller's stack frame.

This could happen even if the ref parameter is const and the function is tail-recursive.

Also, pure functions cannot use that construct.

@tsbockman
Copy link
Contributor Author

ping @TurkeyMan Could you please take a quick look at this, and let me know if it is at all useful to you?

I made it because of the rval->ref const(T) thread you started a couple of days ago, but you never replied to me or anyone else in that thread.

No one else seems very enthusiastic about this PR so far, so if you aren't interested then I'll just close it.

}

if ((p + 1) < names.length)
append(", ");

Choose a reason for hiding this comment

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

if(p>0)append(", "); at the start of the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-random-contributor Thanks.

I'm not sure why I didn't do that in the first place; I've used that idiom before.

@tsbockman
Copy link
Contributor Author

I'll just close this now since no one else seems particularly interested.

@tsbockman tsbockman closed this Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants