-
-
Notifications
You must be signed in to change notification settings - Fork 747
Added std.range.orElse which provides a default value for empty ranges #5154
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
Conversation
|
Please note that this comment is to continue a discussion here. You're confusing a seed with a fallback. Seed is a term used when referring reduction functions, e.g. Furthermore, though technically A fallback is a default value returned when an operation received an input that would otherwise cause it to fail, as opposed to throwing an exception or (e.g. when the assert in My However - this isn't necessarily an argument against including a well-implemented function serving this purpose. (Though I don't think |
This cries to me to call it "fallback" or "orElse" because that is what it does. "seed" doesn't tell me anything - all I can think of is random number generators. |
I really like the name |
Shamelessly stolen from Scala Option ;) |
8168971 to
a1e700f
Compare
|
Phobos-idiomatic D code is going to be a language for the initiated. We need to think about rite of Passage |
Something like that: a candidate should repent about all accidents with Go. Then he should bring bones of three Scala engineers and read Stroustrup: The C++ Programming Language all night without rest. Then we will exam this persons about Phobos internals |
andralex
left a comment
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.
This is a terrific idea. I preapprove the name addition. Thanks!
| Returns: if the range is non-empty the original range and otherwise a range of length | ||
| with the fallback. | ||
| */ | ||
| auto orElse(Range, RangeElementType = ElementType!Range)(Range r, RangeElementType fallback) |
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.
Why is there a need for a default in the second template parameter?
| { | ||
| static struct OrElseRange | ||
| { | ||
| private |
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'm edging close to preferring individual private with declarations because:
- labels (
private:) affect things at distance and make reading and reviewing more complicated - scopes (like here) force an extra indent level
- refactoring/blame is by far best with individual labels
- not a matter of scaling (number of symbols is tractable)
| { | ||
| private | ||
| { | ||
| Range r; |
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.
Traditional name in decorating ranges: source.
| } | ||
| } | ||
|
|
||
| return OrElseRange(r, fallback); |
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 suggest a dramatically simpler and smaller implementation. Use r.chain(fallback) as the range type, and pair it with a bool that tells you whether you need to output the last element.
Sadly we don't have a primitive akin to Unix head (http://www.computerhope.com/unix/uhead.htm), That utility, if passed a negative number, outputs lines except a last few. If we did have that, the implementation would be a one-liner:
return r.chain(only(fallback)).head(r.empty ? -1 : ssize_t.max);Anyhow, using chain should simplify matters quite a bit.
|
One more thing: |
|
From the NG:
To bring this a bit into context: mir-algorithm uses C++-like iterators to avoid this - an example: http://docs.algorithm.dlang.io/latest/mir_ndslice_iterator.html |
|
|
||
| auto opSlice() { return this; } | ||
|
|
||
| string toString() |
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.
This is odd. Custom toString definitions aren't the norm in phobos because most printing functions recognize ranges
|
I find this claim surprising. Replacing a virtual call having 2-7 possible landing sites with a switch or cascade of if/else statements is a well known and venerable optimization, which to my knowledge has been first documented by a few seminal papers in the 90s, notably:
The optimization called "guarded devirtualization" or "type casing" is commonly taught in graduate classes in compiler design and replaces essentially code that you claim is fast, with code that you claim is slow. The win comes from replacing indirect calls (not inlinable and difficult to speculate) with branches and regular calls (easy to inline and speculate). Of course, this technique, although considered canon, is of venerable age and may have become stale due to recent advances in CPU technology. However the entire HHVM team has used it in recent times systematically in their JIT PHP compiler implemented in C++. Replacing virtual calls with typecased switch statements has always been fruitful, and can be seen everywhere in the code. (I also found an easy to digest recent independent benchmark: http://dimitri-christodoulou.blogspot.com/2011/09/case-for-replacing-polymorphism-with.html.) For Regarding |
| else | ||
| alias UT = RetType; | ||
|
|
||
| bool hasFallback; // if the default fallback should be used |
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.
Better name: useFallback
For simple benchmarks orElse maybe faster then vtable. But it ads if conditions while vtable just replace pointers. For example complex pipeline based on The advantage of devirtualisation is that it is done by optimiser after virtual tables were constructed.
stack function was added to import mir.ndslice.allocation: slice;
import mir.ndslice.topology: iota;
// 0, 1, 2
// 3, 4, 5
auto a = iota(2, 3);
// 0, 1
// 2, 3
auto b = iota(2, 2);
// 0, 1, 2, 3, 4
auto c = iota(1, 5);
// 0, 1, 2, | 0, 1
// 3, 4, 5, | 2, 3
// ---------------
// 0, 1, 2, 3, 4
// construction phase
auto s = stack(stack!1(a, b), c);
// allocation phase
auto d = s.slice;
assert(d == [
[0, 1, 2, 0, 1],
[3, 4, 5, 2, 3],
[0, 1, 2, 3, 4],
]);To work with The simplest performance optimisations that can be added to Phobos are
I think that we should move forward with single random access range declaration (ndslice) and have separate implementation for random access ranges and non-random access ranges. I propose to freeze Phobos additions and add only improvements and bugfixes. Other my thoughts about Phobos you already know :-) |
That doesn't sound right. Composition with virtual functions (Java-style) or indirect calls (Haskell style) adds one extra indirect call per composition, which are notoriously difficult to optimize. Do you have any evidence to support the claim that you can compose freely with virtuals and have them all gone by the time the code is run?
But that's a different design than what you discussed previously.
Yah, range methods that specialize certain generic functions would be good to add. I recall you did have a pull request to that effect, what came of it? |
I mean D interface style.
Nope, we can not remove all virtual calls without advanced JIT, but we can reduce pipeline of virtual calls to single one in expression like
A PR was for map specialisation. I implemented our ideas for ndslice iterators and fields. |
Citation: |
|
@9il yah the relative tradeoffs of internal and external iteration are well trodden ground. A good read: http://gafter.blogspot.com/2007/07/internal-versus-external-iterators.html (and of course the GoF book). @wilzbach phobos uses external iteration, but you'd do good here to improve some cases by having void each(alias fun)()
{
if (useElse) fun(other);
else source.each!fun;
}This composes properly and makes for a simple pass through everything. Thx! |
|
ping @wilzbach |
|
I wonder whether or not a |
|
@trikko what would be the semantics of |
|
Just like in SQL. coalesce(a, b, c, d, ...) select the first non-null. I would select a.front if a is a range or only(a).front if not. |
|
Isn't this rather like |
|
@klickverbot it's a conditional chain |
|
More like chain(…).front with automatic application of |
|
Isn't coalesce just a orelse with more than two args? |
|
I'm certainly no expert in D, but is there too much wrong with the following implementation of orElse?: But you lose a few things so maybe that's not a viable alternative. From the unittests you defined, the following fails:
So Maybe an alternative is to have orElse implemented with chain, and "fix" chain to offer opSlice if all input ranges have slicing, etc? Also in the implementation of chain I see an attempt to handle PS: first post here for me in D's github, so I claim the right to ignorance on D's internals and open source directions incase the above is just way too silly ;) Edit: Wait or isn't this just: |
a1e700f to
01670f5
Compare
|
Thanks for your pull request, @wilzbach! 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 fetch digger
dub run digger -- build "master + phobos#5154" |
|
PR is ancient. Closing as stalled. |
As mentioned on #5153 if an API method could potentially return an "undefined response (aka
null), there are a couple of strategies:enforce(aka exceptions)@nogcnowadaysseedstd.algorithm.iteration.maxElementAs the third way seems to be the current strategy (e.g.
fold,reduce, ...), it creates a lot of "redundant" code. Here I propose to add aseed(<range>, <defaultValue>)method that can be combined with any range method to solve this problem. The gist is very simple:The simplest way to add such a
orElsemethod would be to combinechooseandonlylike this:However this has a couple of problems:
static if (!hasDefaultValue!Range) enforce(!r.empty)choosedoesn't work in CTFE (#14660)@safe(it usesunionandvoid[])constelementsopSliceoverloads (e.g.InfiniteRanges)toStringfor user convenience as that's genericonlythat allows assigns, otherwisehasAssignableElementsandhasSwappableElementswon't work.While most of these points are solvable with e.g.
template mixins, I am not sure about all of them and hence this PR contains its an implementation of aOrElseRange.Destroy this idea!
edit: renamed from
seedtoorElse