-
-
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
Add new algorithm: either() #3395
Conversation
|
2 things to note:
|
| { | ||
| if (result) | ||
| { | ||
| results ~= result; |
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.
These array appends would be highly problematic for the performance of a parser...
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.
Done. I return fixed length bool array instead.
|
@nordlow Please do not merge with master, do: # if upstream is dlang/phobos remote
git pull --rebase upstream master |
|
I just realized that it's better for
if (const hit = haystack.tryEvery(needles...)
{
// do something with `hits.count`
}I prefer the last alternative as it will harmonize with the interface of the pending What do you guys think? |
| impossible to mark either() as nothrow. Issue at | ||
| https://issues.dlang.org/show_bug.cgi?id=12647 | ||
| */ | ||
| CommonType!Ts either(Ts...)(lazy Ts a) if (a.length >= 1) |
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.
Nice, two things:
- Could you please try whether it's possible to accept/return by reference? I.e.
either(a[0], a[1]) = 1;should work. But I don't think auto ref cooperates nicely with lazy. - A predicate would help, e.g. choose either non-empty string:
auto s = either!(s => !s.empty)(param, "default");. In fact I think such use would be most useful.
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.
You need to ensure here that CommonType!Ts is not void.
|
I'd like to propose a different implementation for CommonType!Ts either(Ts...)(lazy Ts a)
if (a.length >= 1 && !is(CommonType!Ts == void))
{
foreach (ai; a)
if (ai)
return ai;
return a[$ - 1];
} |
|
So Yes,
|
+1 I'd also point out non-obvious conventions in its usage. |
8c5c1ec to
12bb64e
Compare
|
Made |
| foreach (ref e; a[0 .. $ - 1]) | ||
| if (e) | ||
| return e; | ||
| return a[$ - 1]; |
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 will return a[$ - 1] even if it's falsy.
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.
So which lvalue should be returned if all parameters bool-convert to false? We can't use CommonType!Ts.init because that's an rvalue. Any ideas on this, @andralex?
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.
It seems that the only sensible thing to do would be to throw an exception or assert, but that seems too heavy for such a small function. Maybe have the user provide a default value? It doesn't really make sense, but I don't see much of an alternative. What do Python and Lisp do when all values evaluate to false?
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.
Python:
assert((0 or False) is False)
assert((False or 0) is 0)
assert((False and 0) is False)
assert((0 and False) is 0)
assert((0 or None) is None)
assert((None or 0) is 0)Emacs Lisp:
(and 0 nil) => nil
(and nil 0) => nil
(or 0 nil) => 0
(or nil 0) => 0
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.
Ah, okay.
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.
IMO the important thing here is to have the same behaviour for l-values as for r-values. If so I see three alternatives here:
- Mimic Python's behaviour (in order to get same behaviour for l-value and r-value returns);
eitherreturns last argument upon notrue-conversioneveryreturns first argument upon notrue-conversion
- If default value is supplied return it upon failure for r-value overloads of both
eitherandevery. For the l-value overload we could force the default value to be an l-value via ref parameter in this case, right? - Throw Exception (if no default value is supplied).
What do you think?
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.
Ping. I'm sensing we need a voting on this matter.
My standing point here is a behaviour that is easy to remember.
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.
should be fine as is
|
The latest commit contains a todo; there's a problem with the conversion trait. It doesn't work with arrays. Is there an alternative trait? |
|
|
||
| TODO: Is inout Conversion!T the correct return value? | ||
|
|
||
| NOTE: Lazy parameters are currently, too restrictively, inferred by DMD to |
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.
It's not just nothrow, it's all function attributes.
edit:
Or not. Huh.
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.
So should I remove this comment for now or tell that no attributes can be inferred?
|
Couple more nits and we're ready to go... again :o). |
504017b to
da6fd86
Compare
|
After my latest changes in the first |
da6fd86 to
97f66d4
Compare
|
Regarding lvalues: I'd say let's just forgo them. |
ec32762 to
f86922f
Compare
IMO, we're ready. |
71e550f to
c91ed83
Compare
| if (alternatives.length >= 1 && | ||
| !is(CommonType!(T, Ts) == void) && | ||
| ifTestable!T && | ||
| allSatisfy!(ifTestable, Ts)) |
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.
Just replace these two lines with allSatisfy!(ifTestable, T, Ts)
|
One last nit. Then I preapprove (it sounds... oxymoronic). |
c91ed83 to
c46a463
Compare
|
Done! |
|
Auto-merge toggled on |
|
thx! |
|
Great! |
|
Just realized that we can make use of CommonType!(T, Ts) either(alias pred = a => a, T, Ts...)(T first, lazy Ts alternatives)
if (alternatives.length >= 1 &&
!is(typeof(return) == void) &&
allSatisfy!(ifTestable, T, Ts))Would that be preferrable in other PRs to reduce redundancy and perhaps number of instantiation tries? |
|
This still isn't passing the predicate to |
|
Should we revert and fix, @andralex ? |
|
@nordlow just create a new PR. Thanks @JakobOvrum |
|
Ok, I can add the |
Thanks to D's lazy parameters this new variadic function
eithersimplify pattern matching.either()is similar to Python/Lisp'sor. Same as Phobos'sanybut returns first parameter value which implicitly convert to true, or last argument otherwise. I guessanycould be modified but that would cause breakage on its return type.I've found real use of
either()in various parsing contexts.I'm very open to changing the namings if you feel they are misguiding.
Note that
eithercurrently cannot benothrowbecause of issue https://issues.dlang.org/show_bug.cgi?id=12647