Fix for 12837: Duration's get and individual unit getters are bug-prone.

[jmdavis]

Experience has shown that most people (at least some of who don't read
the documentation) seem to expect that get and the indivdual getters
return what total returns, causing bugs. So, this commit adds partial to
replace get (leaving get as deprecated) and deprecates the individual
unit getters, forcing folks to use total or partial explicitly. It's
likely that most code that will have to change because of these changes
is broken anyway, so this will catch bugs.

Okay. Here's another go at it. Based on the discussion in the previous pull, I renamed getOnly to partial, since while it's a bit obtuse, it goes well with total, and it's shorter than getComponent, which gets a bit long when you have to add the template argument to it. I also adjusted the deprecation messages based on David's comments.

I feel a bit funny about partial being a property, since it takes a template argument, but total has been that way for some time now, and their names are property names, not function names. But we could change them both to be non-property functions without breaking code (it just would make their names a bit odd, since they're nouns rather than verbs).

Regardless, I think that these changes (or something very close to them) need to be made, since the current situation with the getters on Duration has turned out to be overly error-prone. Fortunately, it's likely the case that most code which needs to convert a Duration to units wants total, so while some code is bound to have to be changed for this, it's probably either relatively little code, or it's mostly code that should have been using total anyway.

Once these changes have been approved and merged, I'll create a second pull request for std.datetime to fix the one function that will get deprecation warnings thanks to this.

[yebblies]

I agree with this. I've made this mistake before.

[dnadlinger]

If it's a property, maybe just part (i.e., the noun) would be the better name? A partial would be an overtone of a string in music…

Oh, and I completely agree that the current API is way too bug-prone.

[jmdavis]

If it's a property, maybe just part (i.e., the noun) would be the better name?

That would work. Also, on the newsgroup, Sonke was proposing only, which Andrei seemed to like, though that's an adjective, not a noun, so it would arguably be a bit weird in that regard. But both are shorter than partial and could be treated as properties like total is, which would help make the API more consistent than it would be with getComponent (which is just too long I think when you take the template argument into account).

A partial would be an overtone of a string in music…

Well, as with many words, partial could mean a lot of things. I don't think that I've heard that one before (though if it's related to string instruments, I don't know that I would have, so I don't play one of those).

[jmdavis]

...though that's an adjective, not a noun

Though actually, now that I think of it, partial is an adjective as well, so I guess that if we're willing to go for partial, then we could go for only if folks preferred that.

[brad-anderson]

I don't really like only for the same reasons I didn't like getOnly (not enough emphasis on it being just part of the duration rather than just the total duration using only the specified units). I'll live if it's chosen but I do like partial better.

[schveiguy]

Should weigh in, since I started the controversy from the last pull :)

partial is a good identifier. part works, but does not look right when you pair it with the template parameter: partial!"seconds" vs. part!"seconds"

I do not like only. It still implies "get the duration only as seconds", which could very well be interpreted as the time converted to seconds. If only was the choice, like getOnly, I think it adds too little to justify a change away from get.

Regarding the individual named functions, I would like to see a rename rather than discard. partial!"seconds" is much uglier than seconds. Maybe secondsPart and then alias the old names as deprecated aliases?

Note that this whole scheme is somewhat flawed -- it's quite often that you need not just the e.g. microseconds less than one millisecond, but perhaps the microseconds less than one second (e.g. timeval). I almost think a different concept is needed.

As it stands now, to build a timeval from a duration, one needs to do:

foo(Duration d)
{
   timeval ts;
   ts.tv_secs = d.total!"seconds";
   ts.tv_usecs = d.partial!"msecs" * 1000 + d.partial!"usecs";
   ...
}

I think we can use a single function to do this. That is clearly an enhancement, but the reason I bring it up is because it may replace this entire concept. If we're deprecating, we should do it once.

Just copying the example I had from the old pull request:

d.split!("seconds", "usecs")(&ts.tv_secs, &ts.tv_usecs)

[schveiguy]

Oh, and these DEFINITELY are properties. This is almost the canonical example of why you would have a read-only property.

[jmdavis]

Regarding the individual named functions, I would like to see a rename rather than discard.

I'd really much rather just get rid of them rather than have the function proliferation, especially if we're considering adding a function which splits it into all of the individual components.

Note that this whole scheme is somewhat flawed

Agreed. It definitely makes sense to add a function which splits it into all of the components (or a requested subset of them). However, I don't know that that makes it so that it doesn't make sense to have a function for getting a single component of the duration. Maybe it does though. I'll have to think about it. It would obviate the need for coming up with a better name though. :)

Oh, and these DEFINITELY are properties. This is almost the canonical example of why you would have a read-only property.

Well, that depends on the name. If it were getTotal, it wouldn't be a property, because it's clearly a verb, whereas properties are nouns (since they emulate variables). I don't really disagree with total being a property, since normally it would be. It's just quite bizarre when paired with the template arguments. Normally, properties don't have explicit template arguments (since variables don't). So, while I'd definitely make it a property if it weren't taking a template argument, it just feels weird to have it be a property with the template argument. But then again, if it weren't a property, you'd end up with something like get on the front of its name, which would make the calls even longer, so that's not terribly ideal either. But total has been a property for a while now (though I don't know if anyone has used it as such given the fact that we don't actually enforce that in the compiler at the moment), so unless we all agreed that having it take a template argument made it a bad idea for it to be a property, we might as well just leave it as-is. And if total is, then partial (or part or only or whatever) should be as well. But then again, if we just ditch that in favor of something like split, then that question goes away too.

[brad-anderson]

Ok, thought about this some more and I think my preference is now.

1. split which returns a struct or tuple (with named fields) (e.g., duration.split.seconds).
2. only!"seconds" (yes, I reversed my opinion here)
3. partial!"seconds". Reading this aloud, it could be confused with getting milliseconds (that is, "partial seconds"). secondsPart avoids this problem though.

In any case, 👍 to all three contenders. I think split offers the most and is the most clear.

[jmdavis]

Okay. partial has now been removed in favor of split, and Duration.fracSec was added to the list of deprecated functions, since it was essentially get for the fractional seconds. The few places in druntime which were affected by this have been updated as well (and I think that they're somewhat cleaner as a result). Other than that, this pull should be pretty much the same as before (though the code required for split is far more complicated than what was there for partial).

[schveiguy]

I think you can rewrite this whole thing to be more efficient/straightforward:

ulong secs;
val.split!("seconds", "nsecs")(&secs, &tin.tv_nsec);
tin.tv_sec = secs > tin.tv_sec.max ? tin.tv_sec.max : cast(typeof(tin.tv_sec))secs;

[schveiguy]

Wow, this is awesome! This is better than I imagined for split. I especially like how you can choose based on the parameters to use the return value instead of giving targets. Excellent work.

I would ask for unit tests enforcing invalid calls to split, other than that LGTM.

Edit: LGTM is for the code, but have some nits on the docs and other things (as I posted).

Possible bad calls:

// should be invalid, because you requested a split to nsecs, but did
// not give a target
split!("seconds", "nsecs")(&secs);

// I think this should be invalid, because the code to support it is convoluted.
split!("seconds", "seconds")(&secs, &secs2);
split!("seconds", "seconds")();

// no reason to support this, it's trivial to rewrite the units in order,
// and rejecting this would cut down on template bloat
split!("seconds", "nsecs", "minutes")();
split!("seconds", "nsecs", "minutes")(&secs, &nsecs, &mins);

// possibly this should be bad to cut down on template bloat,
// but I'm not sure.
split!("seconds")(&secs); // error, use total!"seconds"
split!("seconds")(); // same error

[dnadlinger]

months has no quotes, but the others do.

[jmdavis]

Good catch. Will fix.

[schveiguy]

I see the fracSecs property is deprecated, but not the FracSec type. I wonder if fracSecs could be not deprecated, but instead of returning FracSec, just aliases to the appropriate split call, and then deprecate FracSec.

[schveiguy]

Can't we enforce this requirement? In other words, you can figure out how big of an integral type you need, and check the target, rejecting at compile time. Not strictly necessary, but would be a nice feature.

[jmdavis]

We could, but I'm willing to bet that some common uses cases then wouldn't work - e.g. when setting timeval's individual parts, the seconds portion would then have to be a long when on a 32-bit system, it would be int.

[schveiguy]

e.g. when setting timeval's individual parts, the seconds portion would then have to be a long when on a 32-bit system, it would be int.

Good point, I agree with that. I suppose we could only do the requirement for the subsequent fractions, not the total one.

[jmdavis]

Wow, this is awesome! This is better than I imagined for split. I especially like how you can choose based on the parameters to use the return value instead of giving targets. Excellent work.

My initial reaction was to make split return a struct, but your suggestion that it take pointers made me try and figure out how to do both, which is definitely more complicated, but I think that it's worth it. The hard part though was trying to figure out how to do two different sets of variadic arguments (one for the template arguments and one for the function arguments). Fortunately, I figured it out, but it wasn't immediately obvious (and maybe I knew how to do it before, but if so, I forgot due to not needing to do it very often).

I see the fracSecs property is deprecated, but not the FracSec type. I wonder if fracSecs could be not deprecated, but instead of returning FracSec, just aliases to the appropriate split call, and then deprecate FracSec.

FracSec is used by SysTime for its fractional seconds.

// should be invalid, because you requested a split to nsecs, but did
// not give a target
split!("seconds", "nsecs")(&secs);

This is invalid but not actually tested for in the unit tests.

// I think this should be invalid, because the code to support it is convoluted.
split!("seconds", "seconds")(&secs, &secs2);
split!("seconds", "seconds")();

Also invalid but not tested for in the unit tests.

// no reason to support this, it's trivial to rewrite the units in order,
// and rejecting this would cut down on template bloat
split!("seconds", "nsecs", "minutes")();
split!("seconds", "nsecs", "minutes")(&secs, &nsecs, &mins);

Also invalid but not tested for in the unit tests.

// possibly this should be bad to cut down on template bloat,
// but I'm not sure.
split!("seconds")(&secs); // error, use total!"seconds"
split!("seconds")(); // same error

This however is considered perfectly valid and is called out in the docs as being valid (mostly to make it clearer how split works when not all units are split out). We could make it invalid, but I don't see much point. It might hurt generic code (though I suspect that while such code is theoretically possible, it's not terribly likely - i.e. code which is handling the units generically and ends up with a case where it's passing one unit). But more importantly, preventing it would require more code in split and wouldn't actually prevent any template bloat, because it would just force folks to use total instead, which is another templated function, so you'd stil be instantiating a templated function anyway. I agree that it doesn't really make sense to call split with a single template argument, but I don't see much point in going to the extra work of preventing it.

[brad-anderson]

$(D get) and $(D total)

[brad-anderson]

👍. It's way more powerful and more clear now. Good work.

[jmdavis]

Okay. I think that I fixed everything that's been mentioned thus far, though I suppose that I should bring up what monarch_dodra complained about in the newsgroup with regards to split and see if anyone has any comments about it here. Specifically, he objected to the fact that split took pointers instead of by ref. I hadn't realized that it was even possible to have ref when using variadic functions, but apparently it is now. So, that raises the question as to whether split should take pointers or by ref.

Personally, I'm divided, because I prefer the pointers, since it's then nice and explicit at the call site, but that does make it so that any code that calls split with arguments is @system or @trusted, which is a definite downside. Technically, I could probably make it accept both but that seems overly complicated to me. If it weren't for the @system issue, I'd definitely say that we go with pointers, but it is rather annoying that @system then ends up getting forced on the caller. Any thoughts on that from anyone?

[schveiguy]

I suppose that I should bring up what @monarchdodra complained about in the newsgroup

I responded in the newsgroup, I think the point of allowing safe code is valid.

2 things:

1. Can we do out instead of ref for variadic args? Because really this is an out mechanism.
2. The version of split that does not take pointers is valid for @safe code.

Hopefully @monarchdodra has some comments here.

[schveiguy]

One other point, using pointers, you could specify some parts you are not interested in.

For example, if you wanted the number of microseconds less than 1 second, the following might be considered valid:

d.split!("seconds", "usecs")(null, &usecs);

[jmdavis]

Can we do out instead of ref for variadic args? Because really this is an out mechanism.

Sure, assuming that out works with variadic parameters like ref does - though there really isn't much point to it beyond documentation, because all of the variables get assigned to anyway. The real question is whether we want it to be taking pointers or by ref/out. I'd go with pointers hands-down if it weren't for the safety issue.

One other point, using pointers, you could specify some parts you are not interested in.

Yeah, but that would then require checking for null on every argument. I would expect it to be cheaper to simply give a dummy variable if that's what you want to do - or you can use the overload that returns a struct and just ignore the fields that you don't want to use.

[schveiguy]

I would expect it to be cheaper to simply give a dummy variable if that's what you want to do

Good point, it's just easier syntax-wise at the call site to pass in null rather than create a dummy variable. You are right though, it makes the generic case less performant.

[schveiguy]

I'd go with pointers hands-down if it weren't for the safety issue.

I'd point out that correctly implemented scope variables would help here. One thing to note, it should be a pure function, which should guarantee that the pointers never escape. Would that change the allowance of taking addresses of the stack variables?

[brad-anderson]

I'd greatly prefer it just use out rather than pointers. I've never liked getopt's use of pointers. getopt may very well have used pointers just because of the previous limitation on variadic ref arguments Jonathan mentioned so I don't think tradition is a good enough reason to avoid a D feature specifically designed for this use case. Unless there is a really good reason to use pointers, please just use out. I don't think showing that a reference is being taken at the callsite is a good enough reason (it'd be kind of nice if callsite ref were supported, perhaps with a compiler flag to make it mandatory but that's a pretty huge change).

Regarding null arguments, another idea (perhaps too convoluted) is to support split!("_", "hours", "_", "seconds")(hours, seconds) which would behave like the proposed split!("days", "hours", "minutes", "seconds")(null, &hours, null, &seconds).

[jmdavis]

Okay, it's been updated to use out.

[schveiguy]

LGTM

[MartinNowak]

Ready to go?

[brad-anderson]

Also LGTM.

[jmdavis]

Ready to go?

I think so. Certainly, there appear to be no more objections.

[schveiguy]

Auto-merge toggled on

[MartinNowak]

Can you add examples on how to translate code to the changelog please :).

[schveiguy]

An interesting objection came up. dur.weeks is actually equivalent to dur.total!"weeks" because there are no units larger than weeks.

I think we still need to deprecate, but the message needs to be changed.

See this discussion in d.learn: http://forum.dlang.org/post/gbaocvqjlwaforacnrcg@forum.dlang.org

[CyberShadow]

Question: Why have the old functions been deprecated in the same release as the introduction of the new functions? There is no grace period unless you disable deprecation messages, so you can't write code that compiles both under 2.065 and 2.066 unless you use -d or -dw.

[yebblies]

Question: Why have the old functions been deprecated in the same release as the introduction of the new functions? There is no grace period unless you disable deprecation messages, so you can't write code that compiles both under 2.065 and 2.066 unless you use -d or -dw.

Isn't that why deprecations were changed to non-fatal by default?

[jmdavis]

Isn't that why deprecations were changed to non-fatal by default?

Yeah. I don't see a lot of point in not immediately deprecating in most cases now that deprecations just print messages rather than prevent compilation. You get a message telling you that you're going to need to change your code, and you either choose to change it then or wait, but your code continues to compile (and if you don't want to change the code yet or see the messages, you can stil use -d). And without the deprecation messages, most people wouldn't even know that they were going to need to change anything.