-
-
Notifications
You must be signed in to change notification settings - Fork 694
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
Fix generate to be correctly implemented range. #4528
Conversation
This somewhat depends on #4511 |
|
||
Params: | ||
fun = is the $(D isCallable) that gets called on every call to front. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Params: section is missing now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, basically said the same thing as Returns section.
Can someone explain to me the failure in the travis build? |
Note: I'm working on an update to changelog... |
import std.traits: ReturnType, functionAttributes, FunctionAttribute; | ||
enum returnByRef_ = (functionAttributes!fun & FunctionAttribute.ref_) ? true : false; | ||
static if(returnByRef_) | ||
ReturnType!fun *elem_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space between required if (
. This is why Travis complains.
Likewise, at least one if and one foreach below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. that is so not clear from the error message 😄
Yeah, I wouldn't pull this until that's settled. We still haven't heard Andrei's opinion. |
I think Andrei and Walter are on the same page for that. Andrei seems stretched thin, so it may be a while. |
What about a unittest that verifies that multiple calls to |
I used generate in solving a Project Euler problem and switching the new implementation in still gave the right answer, so LGTM. |
@quickfur added |
@@ -327,6 +329,15 @@ static assert( isFinal!(C.ff)); | |||
------- | |||
) | |||
|
|||
$(LI $(LNAME2 generate, `std.range.generate` fixed to be a proper range.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a note here about following the clarified range rules with a link, when #4511 is pulled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most expect ranges to behave this way, the updating of the rules is just a formal presentation of what everyone already knows. So I don't think a link is necessary.
LGTM. |
ping, #4511 has been merged. |
foreach loop. | ||
A by-value foreach loop means that the loop value is not $(D ref). | ||
The resulting range will call $(D fun()) on construction, and every call to | ||
$(D popFront), and the cached value returned when $(D front) is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is/will be returned"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, this grammar error "will be" fixed...
nits fixed. |
|
||
Returns: an $(D inputRange) that returns a new value generated by $(D Fun) on | ||
any call to $(D front). | ||
Returns: an $(D inputRange) where each element represents another call to fun. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backticks over $(D )
throughout
LGTM sans comments |
Done. |
I meant backticks in your changes, but w/e :) |
My OCD forbade that idea :) |
travis fails |
Restarted. I fixed that issue already in another PR I think. |
Please squash the relevant parts; I'll merge then. |
@klickverbot done |
Auto-merge toggled on |
Current coverage is 88.68% (diff: 100%)@@ master #4528 diff @@
==========================================
Files 121 121
Lines 73827 73850 +23
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 65471 65494 +23
Misses 8356 8356
Partials 0 0
|
A bit more complicated to deal with the possibility of ref returns. Also added a unit test to ensure ref returns work.
See rationale here: https://forum.dlang.org/post/nipa43$k0i$1@digitalmars.com