Struct Returned findSplit*() with implicit bool conversion#3288
Struct Returned findSplit*() with implicit bool conversion#3288andralex merged 1 commit intodlang:masterfrom
Conversation
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
I don't think this should be public. There should never be a need for the user to create instances of FindSplitResult; they can already use Tuple in these cases.
edit:
Ditto for the others.
|
A documented unittest example that illustrates the benefits gained from this patch would be appreciated. |
It may be possible to support the subtyping as lvalue by redesigning the implementation of edit: I think the structs should be public and documented, though. It would be the natural way to document that they are implicitly convertible to |
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
This breaks compatibility. Using a subtype of Tuple!(R1, R1, R2) should be acceptable, but inserting the names is not. I think the rest of the PR does pull its weight.
There was a problem hiding this comment.
Tuple!(R1, "pre", R1, "separator", R2, "post") is in turn a subtype of Tuple!(R1, R1, R2), so it should be backwards-compatible. The issue is that this subtyping uses a reinterpret cast which precludes CTFE.
|
Removing the member names from structs made Two things now remain:
Guidelines for these are welcome. I'm a newbie in this regard. BTW: Can you think of any other existing Phobos' algorithms that could benefit in this same regard? |
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
Make these guys Voldemort types inside the respective functions. In the documentation mention "returns a subtype of Tuple!(...) with opCast defined for bool" etc. Then we should be good to go.
|
So it seems voldemort + docs - convenience = win. |
|
Should I use auto pre = longPreExpr;
auto sep = longSepExpr;
auto post = longPostExpr;
return FindSplit!(typeof(pre), typeof(sep), typeof(post))(pre, sep, post);to reduce source code size? |
|
Should I change the names of all the structs to And |
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
This isn't really pattern matching. It might be better to say something like "this enables the convenient idiom shown in the following example".
|
It seems a little redundant to define the exact same struct three times. Maybe use a mixin template or make the structs non-Voldemort types? |
|
Yes, |
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
No need for these to be templates.
No need for these constructors, just use Result(tuple(....)).
There was a problem hiding this comment.
Oh I was mistaken about the first point. You may want to use distinct names from R1 and R2 as they might get confusing.
There was a problem hiding this comment.
I don't understand you last comment:
I'm guessing you mean that I should keep them templates but change the naming of template parameters R1, R2, and R3 so they don't conflict with template parameters of the outer scope.
Am I right?
There was a problem hiding this comment.
Changed the parameters from Rx to Sx.
|
Ping. |
std/algorithm/searching.d
Outdated
There was a problem hiding this comment.
A documented unittest would be superb...
There was a problem hiding this comment.
Should I remove the Example in this comment, then?
There was a problem hiding this comment.
AFAIK documented unittest will be part of example section auto-magically with the added benefit of being tested. See around Phobos the idiom must be applied often enough.
There was a problem hiding this comment.
With documented unittest do you mean that I should put the documentation in line comments?
There was a problem hiding this comment.
No, documented unittest means writing a unittest block with a ddoc comment preceding it. The compiler will insert the body of the unittest into the generated ddocs. For example:
/// My awesome function
auto myFunc(R)(R range) { ... }
///
unittest
{
// The code here will be part of the generated ddocs.
}
|
LGTM the duplication is ugly though. |
|
Would any code rely on the return type of |
|
@MetaLang: Do you want me to check for backwards compatibility? |
I'd like to see it too. |
|
And please squash all commit into one |
|
Unfortunately this breaks with compatibility of auto a = "Carl Sagan Memorial Station";
auto r = findSplit(a, "Velikovsky");
import std.typecons : isTuple;
static assert(isTuple!(typeof(r.asTuple)));
static assert(!isTuple!(typeof(r)));What to do? |
|
@nordlow how about alias-this Tuple!(T) not just T ? |
|
In Tuple!(S1, S1, S2) asTuple;
alias asTuple this;Can you be more precise on what you mean by: Tuple!(T) not just T ? |
Ehm I thought it wasn't. NVM |
|
We may be looking into generalizing |
|
Should that generalization of |
|
@nordlow there is no hurry since 2.069 is at least 2 months away but I'd much prefer to have a pull hanging around by the time this pull is merged. |
|
Ok. Tell me when. |
Pick any time from today to ~ October, 3rd ;) |
|
Ahh, you mean you want me to look into generalizing If so, ideas anyone, on how to accomplish this? |
Yup, I have no plans to extend it for now. By the look of it you should somehow trigger implicit conversion: template isTuple(T)
{
static if (is(Unqual!T Unused : Tuple!Specs, Specs...))
{
enum isTuple = true;
}
else
{
enum isTuple = false;
}
}I'd try __compiles/is(typeof(...)) with a local function that accepts Tuples with the above check, something like: is(typeof((T t){
void f(Spec...)(Tuple!Spec tup){ }
f(t);
}))} |
nordlow
force-pushed
the
struct-returned-findSplit
branch
2 times, most recently
from
eceef8a to
4232b4d
Compare
|
I relaxed the definition of I added an assert for checking that return types of AFAICT, things should work now. An alternative approach which I couldn't figure out the syntax for is enum isTuple(T) = isImplicitlyConvertible!(T, Tuple!Specs, Specs...); |
nordlow
force-pushed
the
struct-returned-findSplit
branch
from
4232b4d to
01e9aa1
Compare
nordlow
force-pushed
the
struct-returned-findSplit
branch
from
01e9aa1 to
100e7a4
Compare
|
@andralex Ping! I believe this is ready now! |
|
@nordlow Try this: It's pretty impressive that D can work backward from the instantiation of Tuple to infer Specs. |
enum isTuple(T, Specs...) = isImplicitlyConvertible(T, Tuple!Specs);fails as |
|
Sorry, I missed the |
|
Sorry... enum isTuple(T, Specs...) = isImplicitlyConvertible!(T, Tuple!Specs);still fails |
|
Ah well, you can't win 'em all. It must be because my custom It's a bit strange that this doesn't work in the first place; it looks like std.typecons.isTuple actually does check for implicit conversions. template isTuple(T)
if (is(Unqual!T Unused: Tuple!Specs, Specs...))
{
//Check if Unqual!T implicitly converts to Tuple!Specs
static if (is(Unqual!T Unused : Tuple!Specs, Specs...))
{
enum isTuple = true;
}
else
{
enum isTuple = false;
}
}Which is exactly what you are checking. I thought that |
|
The plot thickens: enum isTuple(T) = is(Unqual!T: Tuple!Specs, Specs...);
unittest
{
struct TupleLike(T...)
{
Tuple!T payload;
alias payload this;
}
static assert(is(TupleLike!(): Tuple!())); //pass
static assert(is(TupleLike!int: Tuple!int)); //pass
static assert(is(TupleLike!(int, real, string): Tuple!(int, real, string))); //pass
static assert(is(TupleLike!(int, "x", real, "y"): Tuple!(int, "x", real, "y"))); //pass
static assert(is(TupleLike!(int, TupleLike!real, string): Tuple!(int, Tuple!real, string))); //fail (due to the inner TupleLike)
static assert(isTuple!(TupleLike!())); //fail
static assert(isTuple!(TupleLike!(int))); //fail
static assert(isTuple!(TupleLike!(int, real, string))); //fail
static assert(isTuple!(TupleLike!(int, "x", real, "y"))); //fail
static assert(isTuple!(TupleLike!(int, TupleLike!real, string))); //fail
}Looks like |
|
So...is this ready, @MetaLang ? |
|
Looks good to me. |
|
Thanks for this work. It's right at the limit of being too clever for its own good, but I did notice that folks are unclear on what to test if the search succeeded. |
|
Auto-merge toggled on |
|
On the contrary, I like this pattern (aside from the need for alias this). It's a good-enough solution to D's lack of flow-based typing, which would be much more complicated to implement. |
Struct Returned findSplit*() with implicit bool conversion
|
Could this reach 2.069? |
|
@nordlow It's too late, the first release candidate for 2.069 is already out. |
|
Ok. No worries :) |
|
IMO, I think it's reasonable to not allow assignment between the two Voldermort types returned from But it's ok for me to allow it if there's a way to allow assignment between them. |
8 participants
This enables the following useful syntax:
instead of current
for the Phobos algorithms
findSplit,findSplitBeforeandfindSplitAfter.See also discussion at: http://forum.dlang.org/thread/etttlwazcguwpaltyrps@forum.dlang.org#post-tbnvyqawcqmccvociwrr:40forum.dlang.org