Annotate with @nogc most of std.datetime.date's API #5529

PetarKirov · 2017-06-30T15:42:50Z

The only controversial part of this PR is in dayOfYear. There I changed throw new DateTimeException(msg) to assert(0, msg), because that prevented a lot of stuff from being @nogc. Even though this technically is a breaking change, I would regard this use of exceptions inappropriate as it is meant to catch a programmer error, rather than incorrect user input. From the programmer's side of view, it is trivial to validate the user's input simply by using std.datetime.date.yearIsLeapYear and comparing with either daysInLeapYear (366) or daysInYear (365).
(Speaking of which, daysInLeapYear and daysInYear should probably be made public.)
Edit: I changed this PR so that exceptions were changed to asserts only internally. The external API still uses exceptions, so there are no backwards incompatible changes now. See 8ef9c2f for more details.

Most of what's left are the constructors and setters, because they throw exceptions when the value(s) is(are) out of range. If @jmdavis is ok with changing the rest of the exceptions to asserts (where appropriate) I'll send another PR and soon std.datetime.date will be completely GC free.

dlang-bot · 2017-06-30T15:42:52Z

Thanks for your pull request, @ZombineDev!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2017-06-30T15:54:08Z

std/datetime/date.d

    {
        immutable int[] lastDay = isLeapYear ? lastDayLeap : lastDayNonLeap;

        if (day <= 0 || day > (isLeapYear ? daysInLeapYear : daysInYear))
-            throw new DateTimeException("Invalid day of the year.");
+            assert(0, "Invalid day of the year.");


There's also the trick/proposal to use Singleton's for exceptions (e.g. https://github.com/dlang/druntime/pull/1710/files).

Does anyone know the exact status of DIP1008? The DIP status was set to " Post-Preliminary Review Round 1" 26 days ago.

@ZombineDev Andrei has been against such breaking changes in the past and the solution that @wilzbach proposed was originally put forward by him.

The problem is that the singleton approach has its own issues and is not really a solution.

Does anyone know the exact status of DIP1008?

AFAIK Walter said it was on hold

I'm very well aware of dlang/druntime#1710. As a matter of fact I intend to resurrect it, but haven't had much free time lately. However, I strongly believe that throwing is not appropriate in this situation.
Breaking changes are a spectrum and in this case no one's code will cease to compile. On the contrary, there are two immediate benefits:

More of std.datetime.date become usable in @nogc nothrow code -> a game enabler

People who have used anti-patterns such as catch (Throwable) or catch (Exception) would finally be able to fix lurking problem in their code.

@andralex, please advice.

I suggest this be moved anyway outside this PR because it changes semantics, whereas everything else doesn't. For the matter in question, it seems excessive to assert(0) on wrong user input. We don't do that in most places.

PetarKirov · 2017-06-30T16:23:40Z

I amended the commit to address CircleCI's warnings about line length.

andralex · 2017-06-30T16:24:48Z

std/datetime/date.d

    {
        immutable int[] lastDay = isLeapYear ? lastDayLeap : lastDayNonLeap;

        if (day <= 0 || day > (isLeapYear ? daysInLeapYear : daysInYear))
-            throw new DateTimeException("Invalid day of the year.");
+            assert(0, "Invalid day of the year.");


I suggest this be moved anyway outside this PR because it changes semantics, whereas everything else doesn't. For the matter in question, it seems excessive to assert(0) on wrong user input. We don't do that in most places.

PetarKirov · 2017-06-30T16:35:37Z

I suggest this be moved anyway outside this PR because it changes semantics, whereas everything else doesn't.

It would not be as simple because, std.datetime.date.Date.dayOfYear is called from std.datetime.date.Date.this, which get called from std.datetime.date.DateTime.this, and so on.

For the matter in question, it seems excessive to assert(0) on wrong user input. We don't do that in most places.

@andralex how about changing this to:

assert(day <= 0 || day > (isLeapYear ? daysInLeapYear : daysInYear),
    "Invalid day of the year.");

Since this is not user-input, but rather a programmer error?

andralex · 2017-06-30T16:45:01Z

It would not be as simple because, std.datetime.date.Date.dayOfYear is called from std.datetime.date.Date.this, which get called from std.datetime.date.DateTime.this, and so on.

Write a separate private function that doesn't throw and use it from both the throwing function and the rest of the API.

andralex · 2017-06-30T16:46:42Z

If the condition was trivial, e.g. day > 0, a case would be easy to build. But now that we need to have people remembering about leap years, it gets in a different class of problems.

…yOfYear * Keep throwing on invalid input in the public API * Use assert for Date.this(), where exceptions are not appropriate

PetarKirov · 2017-06-30T17:40:22Z

@andralex I think I've addressed your request. Is 8ef9c2f what you had in mind?

andralex · 2017-06-30T17:50:22Z

@ZombineDev close enough but now that you asked it strikes me as overengineered. The path of least resistance is:

// returns whether the input worked or not
private bool setDayOfYearImpl(int day) { ... }
... use it ...

PetarKirov · 2017-06-30T17:57:32Z

@andralex But, now I would need to duplicate the checking of result on all call sites...

andralex · 2017-06-30T18:04:24Z

@ZombineDev point taken

PetarKirov · 2017-06-30T18:06:21Z

@ZombineDev point taken

So @andralex, any other requests, before this is good to go?

PetarKirov · 2017-06-30T18:09:48Z

Thanks for the quick review, @andralex!

wilzbach · 2017-06-30T18:12:26Z

andralex added the auto-merge label

This PR isn't even two hours old. Only trivial PRs should be merged quickly. Otherwise we should try to give reviewers appropriate time to comment.
In this case I think we should at least wait for @jmdavis - after all, he is the official "maintainer" of std.datetime.

andralex · 2017-06-30T18:15:04Z

I think we're good here - it's just adding attributes.

wilzbach · 2017-06-30T18:21:44Z

I think we're good here - it's just adding attributes

No, we add a hack to setDayOfYear to allow it to be @nogc

PetarKirov · 2017-06-30T18:22:19Z

@wilzbach any better suggestions on setDayOfYear?

No, we had a hack to setDayOfYear to allow it to be @nogc

I agree that it looks sub-optimal, but given the backwards compatibility constraints, I think anything different would require more complexity for no gain.

andralex · 2017-06-30T18:24:37Z

@wilzbach my understanding is the change is backward compatible and legit. So the short of it is I gave the PR a review and accepted it. It doesn't have to roast any longer unless the review was superficial. Move fast!

wilzbach · 2017-06-30T18:55:29Z

I agree that it looks sub-optimal,

That's exactly why I want to wait a bit and give other people time to propose better solutions

my understanding is the change is backward compatible and legit.

It's a precedent in the @nogc migration efforts and if I understand @ZombineDev correctly this precedent would be applied to a lot more APIs in the future.

Move fast!

At Phobos alone there are 110 other PRs in the queue - many solely depend on your approval for new symbols. Please let's not waste more time discussing why we shouldn't merge (dangerous) precedents within two hours and no time for anyone to object. Don't worry - I will personally ensure that this PR finds its way in if there are no objections within a reasonable review time frame.

PetarKirov · 2017-06-30T19:45:51Z

I agree that it looks sub-optimal,

That's exactly why I want to wait a bit and give other people time to propose better solutions

To clarify: I think that anything but removing exceptions is sub-optimal. (But that's a topic for the newsgroup.) However, given the backwards compatibilty constraints, I think that my solution is optimal (otherwise, I wouldn't have proposed it).

At Phobos alone there are 110 other PRs in the queue - many solely depend on your approval for new symbols. Please let's not waste more time discussing why we shouldn't merge (dangerous) precedents within two hours and no time for anyone to object. Don't worry - I will personally ensure that this PR finds its way in if there are no objections within a reasonable review time frame.

I can't object giving people more time to review - this can only make the final result better and I'm all for this.

However @wilzbach I really don't understand why you think that this is setting a "dangerous precedent". Given that there's no breaking changes (but only a small internal optimization), how do you think we can do better? What's so dangerous about this PR? (Adding Singleton to druntime is on my to do list, but in this case using exceptions internally is totally inappropriate.)

andralex · 2017-06-30T20:21:21Z

I can't object giving people more time to review - this can only make the final rult better and I'm all for this.

yup yup point taken @wilzbach thx

jmdavis · 2017-06-30T21:20:30Z

I'm completely against changing the public API so that it doesn't use exceptions. However, changing the internals to use a helper function that asserts instead of having the caller catch Exception and assert 0 definitely seems like an improvement. It does arguably complicate things slightly, but having to catch Exception and assert 0 is always ugly even if you aren't concerned about @nogc, and if it's done in enough places, having the extra, helper function even manages to be shorter.

I don't know why all of the attributes are getting rearranged here though. I guess that it doesn't really matter, but I'd used a consistent ordering throughout std.datetime and this screws with that for no apparent reason.

As for merging stuff quickly, I think that we need to be careful about that, much as we don't want to leave stuff sitting around like we too often do either. Some truly trivial stuff is likely fine, but occasionally, something seemingly simple that someone thought was fine and merged quickly turned out to be a big problem for one reason or another, and there have been complaints in the past about PRs that are merged in less than 24 hours.

PetarKirov · 2017-06-30T22:35:41Z

I don't know why all of the attributes are getting rearranged here though. I guess that it doesn't really matter, but I'd used a consistent ordering throughout std.datetime and this screws with that for no apparent reason.

Since I used a semi-automated approach (vim), consistency was important for me too. I noticed that you placed function attributes in the order [@safe] [pure] [nothrow] [@nogc] and I've kept it that way. The places where I moved the function attributes before the functions, was because after I submitted the PR, CircleCI complained that the line length was more than 120 characters, and I chose to move them all in front of the functions, rather than having half of the attributes before the function and half after.
As for const member functions I decided to keep const after the function (since const-ness does not vary depending on the implementation like the other function attributes) and then to try to add after that the largest subset of [@safe] [pure] [nothrow] [@nogc], until the code compiled. So that probably explains why you see const and @safe being rearranged in the diff.
Grepping at std/datetime reveals that before this PR the ratio is close to: 1 : 1.33:

$ grep -Hnr ') const' std/datetime/ | wc -l
113

$ grep -Hnr ') [@np].*const' std/datetime/ | wc -l
151

While after this PR the ratio gets closer to 1: 0.5:

$ grep -Hnr ') const' std/datetime/ | wc -l
174

$ grep -Hnr ') [@np].*const' std/datetime/ | wc -l
90

So it seems the majority of code in std/datetime would move to the [const] [@safe] [pure] [nothrow] [@nogc] style, if this PR is merged. To be honest, even though I have slight preference for [const] [@safe] [pure] [nothrow] [@nogc] over [@safe] [const] [pure] [nothrow] [@nogc], I don't care much about that part of the style, so I would really prefer not getting into bikeshedding and endless discussion about it and having to go through my code and changing it again.

jmdavis · 2017-06-30T22:59:34Z

I don't care much about that part of the style, so I would really prefer not getting into bikeshedding and endless discussion about it and having to go through my code and changing it again.

I brought it up, because I don't really like seeing all that rearranged and would prefer that that sort of thing generally be left alone, particularly when changing it doesn't actually do anything useful, but I approved the PR rather than asking for changes precisely because I recognize that it would be annoying to go back and change the PR.

If no one brings up any issues with this PR, then I'll merge it tomorrow.

wilzbach

Sorry for the holdup - I saw the PR on my phone and overlooked that setDayOfYear is private, so there's nothing controversial about this PR.

wilzbach · 2017-06-30T23:10:55Z

I noticed that you placed function attributes in the order [@safe] [pure] [nothrow] [@nogc] and I've kept it that way.

FWIW at some point we might want to agree on an idiomatic ordering on the attributes (e.g. alphabetical, attributes without @ first, ...). However, I guess this might be very controversial, so I am not really keen on pushing this.

andralex · 2017-06-30T23:11:54Z

Alpha

On Fri, Jun 30, 2017 at 7:10 PM Sebastian Wilzbach ***@***.***> wrote: I noticed that you placed function attributes in the order ***@***.*** <https://github.com/safe>] [pure] [nothrow] ***@***.*** <https://github.com/nogc>] and I've kept it that way. FWIW at some point we might want to agree on an idiomatic ordering on the attributes (e.g. alphabetical, attributes without @ first, ...). However, I guess this might be very controversial, so I am not really keen on pushing this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAill0f1llx4T7GlL--OCzY4QS1TGxoDks5sJYCAgaJpZM4OKvt-> .

-- Please excuse terseness and typos

jmdavis · 2017-06-30T23:24:42Z

However, I guess this might be very controversial, so I am not really keen on pushing this.

I don't see much point in pushing consistency with the order everywhere either, since it's just one more thing that everyone is going to screw up and have CircleCI yell about when it really doesn't matter. It's just that I tried to be consistent with it in std.datetime, and so having it rearranged when there's no technical reason to do so is kind of annoying. I'm pretty sure that ddoc already rearranges them in the actual docs though.

wilzbach · 2017-07-01T00:02:11Z

Alpha

dlang/dlang.org#1784

I'm pretty sure that ddoc already rearranges them in the actual docs though.

Nope, it doesn't:

https://dlang.org/phobos-prerelease/std_datetime_date.html

I am not really keen on pushing this.
I don't see much point in pushing consistency with the order everywhere either, since it's just one more thing that everyone is going to screw up and have CircleCI yell about when it really doesn't matter.

As said, I am not going for push this. The question/ida was really just to define it in the DStyle, s.t. if in doubt, a common "idiomatic" ordering can be picked.

jmdavis · 2017-07-01T02:18:46Z

Nope, it doesn't:

Actually, looking at that, it definitely does. For one, it always puts the attributes on the left-hand side of the signature, regardless of which side they're on in the code. And for another, take a look at julianDay:

https://github.com/dlang/phobos/blob/master/std/datetime/date.d#L2800

has @property on the left of the function and const @safe pure nothrow @nogc on the right, whereas

https://dlang.org/phobos-prerelease/std_datetime_date.html#.DateTime.julianDay

has const pure nothrow @nogc @property @safe. @property was put after the attributes that were on the right, and @safe was randomly grabbed from the middle and put on the end. And the currently released version of the docs

http://dlang.org/phobos/std_datetime.html#.DateTime.julianDay

has the same order (except without the newly added @nogc): const pure nothrow @property @safe - and that's in spite of the fact that this PR rearranged the attributes in the source code. So, ddoc definitely does at least some reordering.

PetarKirov · 2017-07-01T06:43:13Z

FWIW, DDox also rearranges them: pure nothrow @property @nogc @safe - https://dlang.org/library-prerelease/std/datetime/date/date.max.html
See also:

long julianDay() pure nothrow @property @nogc @safe const

https://dlang.org/library-prerelease/std/datetime/date/date.julian_day.html

CyberShadow · 2017-07-01T17:32:24Z

Please let's not waste more time discussing why we shouldn't merge (dangerous) precedents within two hours and no time for anyone to object.

dlang/dlang-bot#117

PetarKirov requested a review from jmdavis June 30, 2017 15:42

wilzbach reviewed Jun 30, 2017

View reviewed changes

PetarKirov force-pushed the datetime-attributes branch from 8694500 to c88f896 Compare June 30, 2017 16:22

andralex requested changes Jun 30, 2017

View reviewed changes

PetarKirov added 2 commits June 30, 2017 20:37

Annotate with @nogc most of std.datetime.date's API

3f8b5b3

Make throwing on invalid input optional in stda.datetime.date.Date.da…

8ef9c2f

…yOfYear * Keep throwing on invalid input in the public API * Use assert for Date.this(), where exceptions are not appropriate

PetarKirov force-pushed the datetime-attributes branch from c88f896 to 8ef9c2f Compare June 30, 2017 17:38

andralex approved these changes Jun 30, 2017

View reviewed changes

andralex added the auto-merge label Jun 30, 2017

wilzbach removed the auto-merge label Jun 30, 2017

JackStouffer assigned wilzbach Jun 30, 2017

jmdavis approved these changes Jun 30, 2017

View reviewed changes

wilzbach approved these changes Jun 30, 2017

View reviewed changes

wilzbach merged commit 404c4c2 into dlang:master Jun 30, 2017

PetarKirov deleted the datetime-attributes branch June 30, 2017 23:26

wilzbach mentioned this pull request Jun 30, 2017

Add attributes ordering to the DStyle dlang/dlang.org#1784

Merged

CyberShadow mentioned this pull request Jul 1, 2017

Wait for PRs to be at least 24 hours old before merging dlang/dlang-bot#117

Open

Annotate with @nogc most of std.datetime.date's API #5529

Annotate with @nogc most of std.datetime.date's API #5529

Conversation

PetarKirov commented Jun 30, 2017 • edited Loading

dlang-bot commented Jun 30, 2017

Bugzilla references

wilzbach Jun 30, 2017

Choose a reason for hiding this comment

JackStouffer Jun 30, 2017

Choose a reason for hiding this comment

PetarKirov Jun 30, 2017

Choose a reason for hiding this comment

andralex Jun 30, 2017

Choose a reason for hiding this comment

PetarKirov commented Jun 30, 2017

andralex Jun 30, 2017

Choose a reason for hiding this comment

PetarKirov commented Jun 30, 2017

andralex commented Jun 30, 2017

andralex commented Jun 30, 2017

PetarKirov commented Jun 30, 2017

andralex commented Jun 30, 2017

PetarKirov commented Jun 30, 2017

andralex commented Jun 30, 2017

PetarKirov commented Jun 30, 2017

PetarKirov commented Jun 30, 2017

wilzbach commented Jun 30, 2017

andralex commented Jun 30, 2017

wilzbach commented Jun 30, 2017 • edited Loading

PetarKirov commented Jun 30, 2017 • edited Loading

andralex commented Jun 30, 2017

wilzbach commented Jun 30, 2017 • edited Loading

PetarKirov commented Jun 30, 2017 • edited Loading

andralex commented Jun 30, 2017 • edited Loading

jmdavis commented Jun 30, 2017

PetarKirov commented Jun 30, 2017

jmdavis commented Jun 30, 2017

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach commented Jun 30, 2017

andralex commented Jun 30, 2017 via email

jmdavis commented Jun 30, 2017

wilzbach commented Jul 1, 2017

jmdavis commented Jul 1, 2017

PetarKirov commented Jul 1, 2017 • edited Loading

CyberShadow commented Jul 1, 2017

PetarKirov commented Jun 30, 2017 •

edited

Loading

wilzbach commented Jun 30, 2017 •

edited

Loading

PetarKirov commented Jun 30, 2017 •

edited

Loading

wilzbach commented Jun 30, 2017 •

edited

Loading

PetarKirov commented Jun 30, 2017 •

edited

Loading

andralex commented Jun 30, 2017 •

edited

Loading

PetarKirov commented Jul 1, 2017 •

edited

Loading