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 14637 - Array operations should work on tuples #6386
Conversation
Thanks for your pull request, @wilzbach! Bugzilla references
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#6386" |
std/typecons.d
Outdated
if (R.length > 1 && areCompatibleTuples!(typeof(this), Tuple!R, "==")) | ||
{ | ||
return field[] == tuple(rhs).field[]; | ||
} |
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 required for static assert(t.fieldNames == tuple("foo", "bar"));
ecb6bcd
to
d8a08e4
Compare
@@ -565,7 +565,7 @@ if (distinctFieldNames!(Specs)) | |||
enum areBuildCompatibleTuples(Tup1, Tup2) = isTuple!Tup2 && is(typeof( | |||
{ | |||
static assert(Tup1.Types.length == Tup2.Types.length); | |||
foreach (i, _; Tup1.Types) | |||
static foreach (i, _; Tup1.Types) |
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.
Since all that's happening in this is a static assert, is there a reason to prefer static foreach
over foreach
?
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.
Style/clarity (see e.g. #5729)
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 all operations that take tuples should use auto ref
seeing as tuple tend to be large.
std/typecons.d
Outdated
@@ -768,6 +770,12 @@ if (distinctFieldNames!(Specs)) | |||
return field[] == rhs.field[]; | |||
} | |||
|
|||
bool opEquals(R...)(R rhs) const |
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.
auto ref may be helpful here
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.
Yeah, but then DMD can no longer find a matching opEquals
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.
Can you file a bug for auto ref opEquals
?
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.
(with the latest constraint + body changes, I can no longer reproduce this -> changed to const auto ref
)
std/typecons.d
Outdated
@@ -768,6 +770,12 @@ if (distinctFieldNames!(Specs)) | |||
return field[] == rhs.field[]; | |||
} | |||
|
|||
bool opEquals(R...)(R rhs) const | |||
if (R.length > 1 && areCompatibleTuples!(typeof(this), Tuple!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.
Does areCompatibleTuples
also include the length check?
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.
Yes (see above)
std/typecons.d
Outdated
bool opEquals(R...)(R rhs) const | ||
if (R.length > 1 && areCompatibleTuples!(typeof(this), Tuple!R, "==")) | ||
{ | ||
return field[] == tuple(rhs).field[]; |
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 wasteful if field[] == rhs.field[]
works, which would be more efficient. Why is there a need to create a temp?
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.
Because R
isn't a tuple - without it I get:
std/typecons.d(776): Error: no property field for tuple (string, string)
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.
Got it, thanks. Well the template constraint would at best describe what the requirements on R are; currently it relies on areCompatibleTuples
, which is utterly undocumented.
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.
std/typecons.d
Outdated
if (op == "~") | ||
{ | ||
static if (isTuple!T) | ||
return Tuple!(AliasSeq!(T._Fields, _Fields))(t.expand, expand); |
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 wonder how this doesn't create an ambiguity when concatenating two tuples.
std/typecons.d
Outdated
if (op == "~") | ||
{ | ||
static if (isTuple!T) | ||
return Tuple!(AliasSeq!(_Fields, T._Fields))(expand, t.expand); |
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 think the use of AliasSeq
here is redundant, but I'm not completely sure without checking it.
std/typecons.d
Outdated
if (op == "~") | ||
{ | ||
static if (isTuple!T) | ||
return Tuple!(AliasSeq!(T._Fields, _Fields))(t.expand, expand); |
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.
Same here with redundant AliasSeq
.
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 like the concept but agree with @andralex's nitpicks, so those should be addressed first.
23153d2
to
0274c34
Compare
std/typecons.d
Outdated
@@ -768,6 +770,16 @@ if (distinctFieldNames!(Specs)) | |||
return field[] == rhs.field[]; | |||
} | |||
|
|||
bool opEquals(R...)(auto ref const R rhs) const |
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.
BTW shouldn't the opEquals
overloads above also use const
?
std/typecons.d
Outdated
@@ -768,6 +770,16 @@ if (distinctFieldNames!(Specs)) | |||
return field[] == rhs.field[]; | |||
} | |||
|
|||
bool opEquals(R...)(const auto ref R rhs) const |
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 think const
is a good idea here:
struct Test(T...)
{
T field;
this (T t) { field = t; }
bool opEquals(R...)(const auto ref R rhs) const
{
static foreach (i, _; T)
if (field[i] != rhs[i])
return false;
return true;
}
}
struct Bad
{
int a;
bool opEquals(Bad b)
{
return a == b.a;
}
}
void main()
{
import std.meta;
auto t = Test!(int, Bad, string)(1, Bad(1), "asdf");
//Error: mutable method Bad.opEquals is not callable using a const object
assert(t == AliasSeq!(1, Bad(1), "asdf"));
}
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.
Urgh. That really sucks. Well, we could duplicate the opEquals
yet another time, but for now I just removed both const
's and added your example to the source code, s.t. we don't regress against this.
Thanks!
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.
Yup. It works if Bad
doesn't define opEquals
and lets the compiler generate it, or if it marks opEquals
as const
or inout
. However, it has to be turtles all the way down; if Bad
wraps another struct Worse
which also has a custom opEquals
, then Worse
must also mark its opEquals
with const
or inout
. This is impossible if Worse
is a struct that you don't control, and is a pain whether you control it or not.
I think the only proper way to do this is to make sure you define an inout
or const
overload of opEquals
iff you define a mutable one.
std/typecons.d
Outdated
@@ -836,6 +848,26 @@ if (distinctFieldNames!(Specs)) | |||
assert(tup1 > tup2); | |||
} | |||
|
|||
/// Allow tuple concatenation |
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.
Params, Returns
std/typecons.d
Outdated
@@ -768,6 +770,16 @@ if (distinctFieldNames!(Specs)) | |||
return field[] == rhs.field[]; | |||
} | |||
|
|||
bool opEquals(R...)(auto ref R rhs) |
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 documented
Added a nice error message if the two concatenated tuples have non-distinct fields. |
Other operations that have been missed?