On 11/18/13 12:59 AM, Jacob Carlborg wrote:
> On 2013-11-18 08:28, Jonathan M Davis wrote:
>
>> That code's like that just because I like to put empty lines before
>> and after
>> if statements and before return statements, as I think that that improves
>> legibility. Short functions like that suffer as a result, because they
>> end up
>> with a larger proportion of the lines being empty than is normal. I'm
>> always a
>> bit torn on that, because I don't like having quite that many empty
>> lines in
>> so few lines, but I also don't like not having space around if
>> statements and
>> return statements. I never feel like I can find a nice, legible
>> balance with
>> short functions like that.
>
> I like the empty newlines as well. Otherwise it feels like reading a
> text without paragraphs.

But three newlines for four lines? That's not paragraphs, it's a grocery list.

Andrei

"Andrei Alexandrescu" <SeeWebsiteForEmail@erdani.org> wrote in message news:l6d9ki$ok6$1@digitalmars.com...
>
> First off, statementsA must not throw. Then, if statementsB may throw, then look at what statement throws. If statement always throws an Error, then accept the code. This is because the scope(failure) effectively translates whatever exception into an Error, and it's legal for a nothrow function to throw Error.
>

Yes.

> Currently it looks like the presence of scope(failure) simply makes the function seem legit, no matter what.
>
> void fun() nothrow
> {
>    scope(failure) {}
>    throw new Exception("so sue me");
> }
>

Yes, obviously a bug.  The compiler is missing the rethrow.

My point was only that try-catch(Exception) and scope(failure) do not have the same semantics.

Am Tue, 19 Nov 2013 04:00:56 +1100
schrieb "Daniel Murphy" <yebblies@nospamgmail.com>:

> "Andrei Alexandrescu" <SeeWebsiteForEmail@erdani.org> wrote in message news:l6d9ki$ok6$1@digitalmars.com...
> >
> > Currently it looks like the presence of scope(failure) simply makes the function seem legit, no matter what.
> >
> > void fun() nothrow
> > {
> >    scope(failure) {}
> >    throw new Exception("so sue me");
> > }
> >
> 
> Yes, obviously a bug.  The compiler is missing the rethrow.

That's severe. It's not even a rethrow, but an independent throw statement sitting squarely in a nothrow function.

-- 
Marco

On 11/18/13 9:00 AM, Daniel Murphy wrote:
>> void fun() nothrow
>> {
>>     scope(failure) {}
>>     throw new Exception("so sue me");
>> }
>>
>
> Yes, obviously a bug.  The compiler is missing the rethrow.

https://d.puremagic.com/issues/show_bug.cgi?id=11542

> My point was only that try-catch(Exception) and scope(failure) do not have
> the same semantics.

Now I got it, thanks. Far as I can tell in this case it doesn't make a big difference, is that correct?

a) with try/catch: Exceptions are transformed in AssertError, Errors just go through

b) with scope(failure): everything is transformed in AssertError.

There is a difference, but both behaviors would be acceptable.


Andrei

On 11/17/13 11:28 PM, Jonathan M Davis wrote:
>> 3. Look into API changes that add nothrow, narrower functions to the
>> more general ones that may throw.
>
> In some cases (e.g. format) that's probably a good idea, but I don't think
> that really scales. In some cases, it would be well worth it, whereas in
> others, it would just be better to use try-catch in the few places where you
> know the function won't throw, because it would be quite rare to be able to
> guarantee that it wouldn't throw. It's also not pleasant to have to duplicate
> functions all over the place.

I thought about this some more. So std.datetime accounts for > 1/3 of all try statements in Phobos. (It also holds a number of other odd records - talking from allegations: largest module in source code, requires most memory to build for unittest, most unittest lines per line of working code. It's great that these are being worked on.) That must mean something. The question is what. It may as well mean that the rest of Phobos is sloppy and does not add nothrow in places where it should, which would make the rest of it insert a bunch of try/catch in the same pattern as std.datetime. An argument based on sense and sensibility would suggest that something ought to be done about that idiom in std.datetime. But the argument "the more nothrow the merrier" is also sensible.

So I think a closer look is warranted here, on a case basis. If the conclusion is that such inspection doesn't scale, fine, we've learned something. But I think there are more valid learnings to derive from here.

I'll walk through a few samples I gathered below, please don't get offended (I know it can feel odd to have one's own code criticized). All in good spirit.

1. Consider:

    this(in DateTime dateTime, immutable TimeZone tz = null) nothrow
    {
        try
            this(dateTime, FracSec.from!"hnsecs"(0), tz);
        catch(Exception e)
            assert(0, "FracSec's constructor threw when it shouldn't have.");
    }

That's because FracSec.from!"hnsecs"(n) may throw for some values of n. To me, this is overkill. Clearly there must be a way to construct a zero time interval without much code being executed at all, let alone checks and exceptions and whatnot. (In particular FracSec.zero looks like the ticket.) Furthermore, having a simpler constructor call a more complicated constructor reminds one of the mathematician who throws away the water in the kettle to regress to an already solved problem.

2. Consider:

    this(in DateTime dateTime, in FracSec fracSec, immutable TimeZone tz = null)
    {
        immutable fracHNSecs = fracSec.hnsecs;
        enforce(fracHNSecs >= 0, new DateTimeException("A SysTime cannot have negative fractional seconds."));
        _timezone = tz is null ? LocalTime() : tz;

        try
        {
            immutable dateDiff = (dateTime.date - Date(1, 1, 1)).total!"hnsecs";
            immutable todDiff = (dateTime.timeOfDay - TimeOfDay(0, 0, 0)).total!"hnsecs";

            immutable adjustedTime = dateDiff + todDiff + fracHNSecs;
            immutable standardTime = _timezone.tzToUTC(adjustedTime);

            this(standardTime, _timezone);
        }
        catch(Exception e)
        {
            assert(0, "Date, TimeOfDay, or DateTime's constructor threw when " ~
                      "it shouldn't have.");
        }
    }

This is not even marked as nothrow (sic!) so the entire try/catch is completely meaningless - probably a rote application of the idiom without minding its intent. The only difference it would make to the user is that a library bug may manifest in slighlty different ways. The code has a smartaleck thing about it: "You'd be surprised to see an exception here. I, too, would be surprised, and I wanted to make sure I tell you about it". The entire try/catch should be removed.

3. Consider:

    this(in Date date, immutable TimeZone tz = null) nothrow
    {
        _timezone = tz is null ? LocalTime() : tz;

        try
        {
            immutable adjustedTime = (date - Date(1, 1, 1)).total!"hnsecs";
            immutable standardTime = _timezone.tzToUTC(adjustedTime);

            this(standardTime, _timezone);
        }
        catch(Exception e)
            assert(0, "Date's constructor through when it shouldn't have.");
    }

Here, again, we have a nothrow constructor call a more general one, which may throw in general. The intent - maximizing internal reuse through forwarding - is noble. The realization - it takes more code to reuse than to just set the blessed variables - is a corruption of the noble goal.

4. Consider:

    @property FracSec fracSec() const nothrow
    {
        try
        {
            auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);

            if(hnsecs < 0)
                hnsecs += convert!("hours", "hnsecs")(24);

            hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);

            return FracSec.from!"hnsecs"(cast(int)hnsecs);
        }
        catch(Exception e)
            assert(0, "FracSec.from!\"hnsecs\"() threw.");
    }

This is an interesting one. Going to FracSec.from's documentation, we see that it throws if the input does not fall within (-1, 1) seconds, which means (if I count correctly) hnsec inputs must fall between -10 million and 10 million. But we already know that we're in good shape because we just called removeUnitsFromHNSecs. The problem is the two calls are disconnected. That means the API could be improved here: combine the two functions by defining a way to get the fractionary FracSec from a time value. That will always succeed by definition, so we're in good shape.

5. Consider:

    tm toTM() const nothrow
    {
        try
        {
            auto dateTime = cast(DateTime)this;
            tm timeInfo;

            timeInfo.tm_sec = dateTime.second;
            timeInfo.tm_min = dateTime.minute;
            timeInfo.tm_hour = dateTime.hour;
            timeInfo.tm_mday = dateTime.day;
            timeInfo.tm_mon = dateTime.month - 1;
            timeInfo.tm_year = dateTime.year - 1900;
            timeInfo.tm_wday = dateTime.dayOfWeek;
            timeInfo.tm_yday = dateTime.dayOfYear - 1;
            timeInfo.tm_isdst = _timezone.dstInEffect(_stdTime);

            version(Posix)
            {
                char[] zone = (timeInfo.tm_isdst ? _timezone.dstName : _timezone.stdName).dup;
                zone ~= "\0";

                timeInfo.tm_gmtoff = cast(int)convert!("hnsecs", "seconds")(adjTime - _stdTime);
                timeInfo.tm_zone = zone.ptr;
            }

            return timeInfo;
        }
        catch(Exception e)
            assert(0, "Either DateTime's constructor threw.");
    }

In fact the assertion message is wrong here. There's no constructor that could fail. The problem is with the .dup! That's a bug in .dup for char[] - it may indeed throw, but that's an Error not an exception.

(Anyhow, I got curious about tm_zone out of concern for the odd allocation. According to http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_19.html it's a gnu extension and it's a const char*, meaning the user code is not supposed to mess with it. Therefore, my guess is some sort of memoization here would be in order. But then http://www.gsp.com/cgi-bin/man.cgi?topic=mktime suggests it's not const. Then http://www.eecs.harvard.edu/syrah/vino/release-0.40/man/programref/libc/time/ctime.3.txt says "he tm_zone field of a returned struct tm points to a static array of characters, which will also be overwritten at the next call (and by calls to tzset)." That means a static char[3] would be in order. Anyhow, more investigation is needed here.)

6. Consider:

    /+ref SysTime+/ void roll(string units)(long value) nothrow
        if(units == "hours" ||
           units == "minutes" ||
           units == "seconds")
    {
        try
        {
            auto hnsecs = adjTime;
            auto days = splitUnitsFromHNSecs!"days"(hnsecs) + 1;

            if(hnsecs < 0)
            {
                hnsecs += convert!("hours", "hnsecs")(24);
                --days;
            }

            immutable hour = splitUnitsFromHNSecs!"hours"(hnsecs);
            immutable minute = splitUnitsFromHNSecs!"minutes"(hnsecs);
            immutable second = splitUnitsFromHNSecs!"seconds"(hnsecs);

            auto dateTime = DateTime(Date(cast(int)days), TimeOfDay(cast(int)hour, cast(int)minute, cast(int)second));
            dateTime.roll!units(value);
            --days;

            hnsecs += convert!("hours", "hnsecs")(dateTime.hour);
            hnsecs += convert!("minutes", "hnsecs")(dateTime.minute);
            hnsecs += convert!("seconds", "hnsecs")(dateTime.second);

            if(days < 0)
            {
                hnsecs -= convert!("hours", "hnsecs")(24);
                ++days;
            }

            immutable newDaysHNSecs = convert!("days", "hnsecs")(days);

            adjTime = newDaysHNSecs + hnsecs;
        }
        catch(Exception e)
            assert(0, "Either DateTime's constructor or TimeOfDay's constructor threw.");
    }

Here again the error message is mistaken; the only culprit is TimeOfDay's constructor. This prompts a different protest. Note that up until the point liable to throw, the input "value" is not involved at all, meaning that whatever simple manipulation is done there cannot be done without an inefficiency being in the loop. So there's a problem with the API design.

7. Consider:

    DateTime opCast(T)() const nothrow
        if(is(Unqual!T == DateTime))
    {
        try
        {
            auto hnsecs = adjTime;
            auto days = splitUnitsFromHNSecs!"days"(hnsecs) + 1;

            if(hnsecs < 0)
            {
                hnsecs += convert!("hours", "hnsecs")(24);
                --days;
            }

            immutable hour = splitUnitsFromHNSecs!"hours"(hnsecs);
            immutable minute = splitUnitsFromHNSecs!"minutes"(hnsecs);
            immutable second = getUnitsFromHNSecs!"seconds"(hnsecs);

            return DateTime(Date(cast(int)days), TimeOfDay(cast(int)hour, cast(int)minute, cast(int)second));
        }
        catch(Exception e)
            assert(0, "Either DateTime's constructor or TimeOfDay's constructor threw.");
    }

This seems to be another instance of the problem at (6). No matter how one looks at it, one must admit that the API should be designed such that a valid SysTime should convert to the corresponding DateTime without much intervening brouhaha.

=============

That 7 issues with as many try/catch instances, i.e. 13% of the total 54. You may think I cherry-picked them; in fact, they are the FIRST 7 instances I found by simply searching the file for 'try'. I have no reason to believe I won't find similar issues with most or all of them.

I think std.datetime needs a pass with an eye for this idiom and obviating it wherever possible.


Andrei

On Monday, November 18, 2013 13:23:23 Jacob Carlborg wrote:
> On 2013-11-18 12:03, Jonathan M Davis wrote:
> > assert(0) is intended specifically for use in cases where a line is supposed to be unreachable, and it wouldn't make any sense to use it in any other case, because assertion failures are intended to kill the program, and assert(0) always fails. It happens that it throws an AssertError in non-release mode in order to give you better debug information on failure, and it's a HLT instruction in release, but in either case, it's intended to be unreachable code.
> > 
> > The spec really should be updated to make it clear that when assertions
> > are
> > compiled in, assert(0) throws an AssertError and that when assertions are
> > supposed to be compiled out, it becomes a HLT instruction. And if need be,
> > we can update the spec to require that try-catches be compiled out when
> > assertions are compiled out, and the catch's body only contains an
> > assert(0).
> Does all architectures support the HLT instruction or equivalent? The spec explicitly says HLT is used on x86.

I don't know. My knowledge of stuff at that level on x86 is already poor. I have pretty much zero knowledge about that sort of thing on other architectures. The extent of my knowledge in that area tends to be restricted to big endian vs little endian. The only reason that I even know that HLT exists is thanks to this feature in D.

If it's not possible to require HLT or an equivalent, then it wouldn't make sense to put that in the spec, which would be unfortunate, but ideally that would be required. However, even if it can't be, that doesn't change the fact that assert(0) is intended to indicate unreachable code and doesn't make sense for anything else (as I recall, the compiler itself inserts it for that purpose at the end of functions in some circumstances to catch missing return statements). So, I see no problem with the compiler making optimizations in release based on the assumption that assert(0) should never be reached.

- Jonathan M Davis

On 11/17/2013 10:32 PM, Andrei Alexandrescu wrote:
> 4. ...?

Revisit why removeUnitsFromHNSecs, etc., are throwing. Since the caller is guaranteeing they don't throw, I suspect the runtime data validation is itself invalid and the throws should be replaced with asserts.

On 11/18/2013 11:44 AM, Andrei Alexandrescu wrote:
> 1. Consider:
>
>      this(in DateTime dateTime, immutable TimeZone tz = null) nothrow
>      {
>          try
>              this(dateTime, FracSec.from!"hnsecs"(0), tz);
>          catch(Exception e)
>              assert(0, "FracSec's constructor threw when it shouldn't have.");
>      }
>
> That's because FracSec.from!"hnsecs"(n) may throw for some values of n. To me,
> this is overkill. Clearly there must be a way to construct a zero time interval
> without much code being executed at all, let alone checks and exceptions and
> whatnot. (In particular FracSec.zero looks like the ticket.) Furthermore, having
> a simpler constructor call a more complicated constructor reminds one of the
> mathematician who throws away the water in the kettle to regress to an already
> solved problem.

I agree. But I'll add that it would be even better to redesign FracSec.from so it never throws at all - have it assert(0) for invalid values being passed to it.

Exceptions should not be thrown for invalid arguments. Data validation should be handled separately.

On 11/18/2013 11:44 AM, Andrei Alexandrescu wrote:
> Here, again, we have a nothrow constructor call a more general one, which may
> throw in general. The intent - maximizing internal reuse through forwarding - is
> noble. The realization - it takes more code to reuse than to just set the
> blessed variables - is a corruption of the noble goal.

I also have a design issue with having throwing constructors at all. I know D allows them, but I consider it a bad practice.

On Monday, November 18, 2013 07:56:15 Andrei Alexandrescu wrote:
> On 11/18/13 12:59 AM, Jacob Carlborg wrote:
> > I like the empty newlines as well. Otherwise it feels like reading a text without paragraphs.
> 
> But three newlines for four lines? That's not paragraphs, it's a grocery list.

Yeah. It falls apart on some level when there are so few lines. It's just that it's also ugly to mush them all together. So, to me at least, it seems like it's ugly either way, and in the past, I've just stuck with my normal pattern of having the blank lines when that's the case. Now (in part, thanks to your complaints about it), I'm more likely to mush the lines together when putting the blank lines would result in a bunch of one liners. But I don't really feel like there's a good solution when the function is so short.

- Jonathan M Davis