Thread overview
Weird interaction of design choices: Duration + sum
Oct 13, 2017
Luís Marques
Oct 13, 2017
jmh530
Oct 13, 2017
Jonathan M Davis
Oct 13, 2017
Jonathan M Davis
Oct 13, 2017
jmh530
Oct 13, 2017
Jonathan M Davis
October 13, 2017
If you do this:

    import std.algorithm : sum;
    import core.time : Duration;

    Duration[] parts;
    auto total = parts.sum;

You'll get an error ("struct core.time.Duration member this is not accessible"). That's because the Duration constructor is private, and `sum` calls `sum(r, Unqual!Seed(0))`. BTW, not the most helpful of messages for beginners, maybe this could be phrased to say the ctor is private?

I suppose that the ctor is private due to the arbitrary nature of its input units (hecto-nanoseconds), even though Duration(0) is unambiguous. On the other hand, sum has to use Seed(0) instead of Seed.init because some types don't use 0 as .init, namely floats. The result is that you have to use range.sum(Duration.zero), or Duration.init.

Something here rubs me the wrong way, but I can't tell exactly what it is, or what can be done about it :)
October 13, 2017
On Friday, 13 October 2017 at 18:36:58 UTC, Luís Marques wrote:
> [snip]

What if it were package instead of private?
October 13, 2017
On Friday, October 13, 2017 19:11:40 jmh530 via Digitalmars-d wrote:
> On Friday, 13 October 2017 at 18:36:58 UTC, Luís Marques wrote:
> > [snip]
>
> What if it were package instead of private?

What, Duration's constructor? Duration is in core.time, whereas sum is in std.algorithm. They aren't in the same package on any level.

IMHO, allowing Duration's constructor to be public is just begging for trouble, which is why it's not public. Time units should be typed, which they are with dur and its aliases. We've tried to strip all untyped time units from druntime and Phobos, and AFAIK, we've managed it. Allowing them is just begging for bugs.

It sounds like that has had an annoying side effect in the case of sum, but it's not like Duration is going to be the only type out there that doesn't allow direct construction but can be used with sum. You just have to provide the seed value yourself.

- Jonathan M Davis


October 13, 2017
On 10/13/17 3:11 PM, jmh530 wrote:
> On Friday, 13 October 2017 at 18:36:58 UTC, Luís Marques wrote:
>> [snip]
> 
> What if it were package instead of private?

This wouldn't help, sum is in phobos, Duration in druntime.

The answer is that sum shouldn't be too clever. IMO, initializing to 0 should be only done if the type's .init value doesn't match 0. Since only floating points and char/wchar/dchar do this, just do the Seed(0) for those, else use Seed.init.

-Steve
October 13, 2017
On 10/13/17 3:24 PM, Steven Schveighoffer wrote:
> On 10/13/17 3:11 PM, jmh530 wrote:
>> On Friday, 13 October 2017 at 18:36:58 UTC, Luís Marques wrote:
>>> [snip]
>>
>> What if it were package instead of private?
> 
> This wouldn't help, sum is in phobos, Duration in druntime.
> 
> The answer is that sum shouldn't be too clever. IMO, initializing to 0 should be only done if the type's .init value doesn't match 0. Since only floating points and char/wchar/dchar do this, just do the Seed(0) for those, else use Seed.init.

I take the last part back a bit. Some custom types may default to NaN-like values, so Seed(0) is necessary.

What should happen is that sum should test if Seed(0) is viable, and if not, use Seed.init.

-Steve
October 13, 2017
On Friday, October 13, 2017 18:36:58 Luís Marques via Digitalmars-d wrote:
> If you do this:
>
>      import std.algorithm : sum;
>      import core.time : Duration;
>
>      Duration[] parts;
>      auto total = parts.sum;
>
> You'll get an error ("struct core.time.Duration member this is
> not accessible"). That's because the Duration constructor is
> private, and `sum` calls `sum(r, Unqual!Seed(0))`. BTW, not the
> most helpful of messages for beginners, maybe this could be
> phrased to say the ctor is private?

Well, that's a general issue separate from sum or Duration specifically. "Not accessible" is more general than talking about private, and it's something that I would expect most C++ or D programmers to know, but I'm not exactly in the best place to know what someone coming from another language would know or find confusing. Certainly, if you feel that specifically indicating the the problem is that the constructor is private would make for a better error message, then feel free to create an enhancement request for it:

https://issues.dlang.org

- Jonathan M Davis


October 13, 2017
On Friday, October 13, 2017 15:28:03 Steven Schveighoffer via Digitalmars-d wrote:
> On 10/13/17 3:24 PM, Steven Schveighoffer wrote:
> > On 10/13/17 3:11 PM, jmh530 wrote:
> >> On Friday, 13 October 2017 at 18:36:58 UTC, Luís Marques wrote:
> >>> [snip]
> >>
> >> What if it were package instead of private?
> >
> > This wouldn't help, sum is in phobos, Duration in druntime.
> >
> > The answer is that sum shouldn't be too clever. IMO, initializing to 0 should be only done if the type's .init value doesn't match 0. Since only floating points and char/wchar/dchar do this, just do the Seed(0) for those, else use Seed.init.
>
> I take the last part back a bit. Some custom types may default to NaN-like values, so Seed(0) is necessary.
>
> What should happen is that sum should test if Seed(0) is viable, and if
> not, use Seed.init.

The other advantage to that is that it wouldn't risk breaking anything (at least, I don't think that it would), whereas even if Seed.init would work for a type that accepted 0 with its constructor, the semantics might not be the same, which could then break code if we just started using Seed.init for those types.

- Jonathan M Davis


October 13, 2017
On Friday, 13 October 2017 at 19:28:03 UTC, Steven Schveighoffer wrote:
>> 
>> This wouldn't help, sum is in phobos, Duration in druntime.
>> 
>> The answer is that sum shouldn't be too clever. IMO, initializing to 0 should be only done if the type's .init value doesn't match 0. Since only floating points and char/wchar/dchar do this, just do the Seed(0) for those, else use Seed.init.
>
> I take the last part back a bit. Some custom types may default to NaN-like values, so Seed(0) is necessary.
>
> What should happen is that sum should test if Seed(0) is viable, and if not, use Seed.init.
>
> -Steve

Ah, I see. Something like:

private enum bool canZeroConstruct(R) = is(typeof(R(0)) == R);

auto sum(R)(R r)
    if (isInputRange!R && !isInfinite!R && is(typeof(r.front + r.front)))
{
    import std.algorithm : sum;
    alias E = Unqual!(ElementType!R);
    static if (isFloatingPoint!E)
        alias Seed = typeof(E.init + 0.0); //biggest of double/real
    else
        alias Seed = typeof(r.front + r.front);
    static if (canZeroConstruct!E)
        return sum(r, Unqual!Seed(0));
    else
        return sum(r, Unqual!Seed.init);
}