Fix Issue 4582 - distinct field names constraint for std.typecons.Tuple#5725
Fix Issue 4582 - distinct field names constraint for std.typecons.Tuple#5725dlang-bot merged 3 commits intodlang:masterfrom
Conversation
|
Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
|
I just had an idea for a shorter and more efficient implementation that piggybacks the compiler's symbol hash table. Try this: enum allDistinct(string[] names) = __traits(compiles,
{
static foreach (name; names)
mixin("enum int" ~ name ~ " = 0;");
});
static assert( allDistinct!([]));
static assert( allDistinct!(["a"]));
static assert(!allDistinct!(["a", "a"]));
static assert( allDistinct!(["a", "b"]));Edit1: I did a quick benchmark: private template Iota(int stop)
{
static if (stop <= 0)
alias AliasSeq!() Iota;
else
alias AliasSeq!(Iota!(stop-1), stop-1) Iota;
}
private bool distinctFieldNames1(T...)()
{
enum int tlen = T.length;
foreach (i1; Iota!(tlen))
static if (is(typeof(T[i1]) : string))
foreach (i2; Iota!(tlen))
static if (i1 != i2 && is(typeof(T[i2]) : string))
if (T[i1] == T[i2])
return false;
return true;
}
private bool distinctFieldNames2(T...)()
{
enum int tlen = T.length;
foreach (i1; staticIota!(0, tlen))
static if (is(typeof(T[i1]) : string))
foreach (i2; staticIota!(0, tlen))
static if (i1 != i2 && is(typeof(T[i2]) : string))
if (T[i1] == T[i2])
return false;
return true;
}
enum bool allDistinct(string[] names) = __traits(compiles,
{
static foreach (name; names)
mixin("enum int" ~ name ~ " = 0;");
});
import std.algorithm.iteration : map;
import std.conv : to;
import std.range : array, iota;
import std.meta : AliasSeq, aliasSeqOf;
import std.typecons : staticIota;
enum gen(size_t n) = n.iota.map!(i => "a" ~ i.to!string).array;
static immutable string[] s = gen!300;
alias s2 = aliasSeqOf!s;
When I increase the the size to 500 (by using
When increasing the size to 10000, Edit2: here's an a bit more optimal enum bool distinctFieldNames3(names...) =
distinctFieldNames3Impl([names]);
private bool distinctFieldNames3Impl(string[] names)
{
foreach (i; 0 .. names.length)
foreach (j; i + 1 .. names.length)
if (names[i] == names[j])
return false;
return true;
}For 1000 I get: Though it scales poorly w.r.t. memory, here's the result for 10000: On the other hand, if there are actually duplicates (tested by changing In comparison, For a direct comparison between
So, it's a close tie. What do you guys think we should pick? |
PetarKirov
left a comment
There was a problem hiding this comment.
Let's use a more efficient implementation - see #5725 (comment).
std/typecons.d
Outdated
| alias sharedToString = field; | ||
| } | ||
|
|
||
| private template Iota(int stop) |
There was a problem hiding this comment.
Ideally, we need to have staticIota with 1, 2 and 3 parameters in std.meta but that's a battle for another day. In this case there already something similar defined in this module, which you should be able to reuse. IIRC, it's more optimal because it uses log(n) instances, instead of n, i.e. it should hit the 500 limit harder.
There was a problem hiding this comment.
There are at least 2 staticIota-a in our std library. Time to draft a public API?
There was a problem hiding this comment.
Incidentally, last night and this morning I was working on a faster version of staticIota. When I'm done, I'll post a PR for std.meta.
There was a problem hiding this comment.
Looking forward! Otherwise you could also move this to someone and make it package(std) for now
There was a problem hiding this comment.
Before static foreach, foreach (const i; aliasSeqOf!(iota(0, n))) was the common pattern, now it's simply:
void main(string[] args)
{
static foreach (i; 0..3)
pragma(msg, i);
}https://run.dlang.io/is/QWxTYt
So thinking about it: at least for this PR we don't need Iota at all..
|
@ZombineDev Your |
|
Yes, I like it better because it shorter and easier to review at a glance. |
|
The only disadvantage is that part of performance comes from committing the check for Edit: with enum bool allDistinct(names...) = allDistinctImpl!([names]);
enum bool allDistinctImpl(string[] names) = __traits(compiles,
{
static foreach (name; names)
mixin("enum int" ~ name ~ " = 0;");
});If you call
And with
And no trace from where exactly the error comes from. |
|
Yes, but that check is important, since otherwise, you end up with valid cases being rejected. For example, a Tuple instantiation : Tuple(int, "one", int, "two") will fail. |
|
This can be solved like this: import std.meta : Stride;
enum bool allDistinct(names...) =
allDistinctImpl!([ Stride!(2, names) ]);But the original issue still stands. |
|
Although I agree that your solution is more elegant I do not think that performance is an issue in this case since Tuple is limited to 500 value names. In terms of readability I find bearophile's code really nice (and to make |
std/typecons.d
Outdated
| private bool distinctFieldNames(T...)() | ||
| { | ||
| enum int tlen = T.length; | ||
| foreach (i1; Iota!(tlen)) |
There was a problem hiding this comment.
static foreach (i; 0 .. tlen)
std/typecons.d
Outdated
| enum int tlen = T.length; | ||
| foreach (i1; Iota!(tlen)) | ||
| static if (is(typeof(T[i1]) : string)) | ||
| foreach (i2; Iota!(tlen)) |
There was a problem hiding this comment.
static foreach (j; 0 .. then)
std/typecons.d
Outdated
| Specs = A list of types (and optionally, member names) that the `Tuple` contains. | ||
| */ | ||
| template Tuple(Specs...) | ||
| template Tuple(Specs...) if (distinctFieldNames!(Specs)()) |
There was a problem hiding this comment.
Put this in a separate line, indent-aligned with template
37c8726 to
4e8fa84
Compare
|
ping @ZombineDev @MetaLang is this ok? |
MetaLang
left a comment
There was a problem hiding this comment.
Even though it's a private symbol it should still have a unit test.
|
@MetaLang I haven't seen any private symbols getting unittested in phobos. How about this : I add a unittest for Tuple which tests that tuples that do not have distinct field names fail to compile. |
|
It depends on what code you've been looking at, probably. Personally, anytime I add a new symbol - public or private - I add unit tests that try to exercise it as thoroughly as possible. If we're not doing that currently in Phobos, we should, because tests will ensure that we know when something breaks. |
Yes. Apart from guaranteeing that the function won't break in the future, they help future reader as documentation - and for your current reviewers as well, of course! |
|
@ZombineDev @MetaLang All should be good now |
|
@RazvanN7 thanks! |
Unfortunately, I don't have permissions to trigger a rebuild on CircleCI. EDIT: never mind, I just had to authenticate via Github. It's running now. |
FWIW the PR to fix this has been open for half a year: dlang/installer#218 |
I also did some testing and it seems that if Tuple has a list of types longer that 500 you get a recursive expansion error due to the fact that dmd does not support to expand more than 500 times [1]. Accordingly I did some measurements with and without the distincFieldNames constraints and it seems that the performance is not affected by this diff. The worst time without this patch was 3,263s and with patch 3,347s. I think that this is a negligible difference considering that the error message will be very helpful this way. Thanks to bearophile_hugs for his solution which is basically copy-pasted.
[1] https://github.com/dlang/dmd/blob/master/src/ddmd/dtemplate.d#L7438