-
-
Notifications
You must be signed in to change notification settings - Fork 706
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 Issue 18374 - Add range functions to Nullable #8417
Conversation
|
Thanks for your pull request and interest in making D better, @Herringway! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8417" |
5bd6872 to
a9f2f89
Compare
a9f2f89 to
fb18e1a
Compare
I suggest grouping all of these functions under the name "Range interface functions" like this: Here's an example in std.regex: https://dlang.org/phobos/std_regex.html#.Captures.front. Also I suggest adding a documented unittest to showcase idiomatic range-based usage. You can borrow a few examples from the optional package: https://code.dlang.org/packages/optional |
|
@atilaneves @pbackus @dkorpel what are your thoughts on this addition? |
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 don't use Nullable, so I don't have a strong opinion, but I think it's fine to give it a range interface.
std/typecons.d
Outdated
| return this; | ||
| } | ||
| /// | ||
| inout(typeof(this)) opSlice()(size_t from, size_t to) inout |
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 get the range interface, but not sure why it needs slicing.
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.
See previous comment re: opSlice.
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 don't know why. I don't know why not, either. Either way, it was simple enough to support.
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.
Prefer less code, it's easier to maintain.
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.
Satisfying hasSlicing!T allows more efficient branches to be taken in several Phobos algorithms:
It's also necessary if we want ranges of Nullables (e.g., chain(nullable(x), nullable(y), nullable(z))) to support slicing.
std/typecons.d
Outdated
| return this; | ||
| } | ||
| /// | ||
| inout(typeof(this)) opSlice()(size_t from, size_t to) inout |
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.
See previous comment re: opSlice.
f890db4 to
734aada
Compare
734aada to
d5ae361
Compare
d5ae361 to
d58dd92
Compare
|
Almost forgot to add a changelog entry. |
|
Very nice, a much welcome change 🎉 |
Let's get Nullable working with the rest of phobos. Is this overkill? Probably. Definitely. 100% yes.
I genuinely have no idea what sort of documentation to write for these functions.