[Lazarus] Bump issue

[R.Smith]

On 2021/01/22 23:58, Marco van de Voort via lazarus wrote:
>
> Op 2021-01-22 om 22:56 schreef Michael Van Canneyt via lazarus:
>>
>>
>> I have some time this weekend, I will commit it.
>
> Is it really a good idea to accept msec=1000 for TryEncodeTimeInterval 
> in a general unit like Dateutils?
>
>
Maybe - can you construe a situation where the allowance would cause 
undue error or miscalculation of a datetime?  It's essentially an 
allowance for rounding. If your date-time calculation tries to add 
999.999ms or 0.9999 sec to a datetime value, would you really be harmed 
if one full second was added? If so, what does it say for the occasion 
where you add 0.1116s and either 0.112s or 0.111s are physically added 
after rounding/truncation - is the prior any worse? Currently rounding 
works in all cases (000.000 up to 999.99444) except the very final 0.5ms 
of the range, and code has to be added to everything to fix that small 
remainig range because rounding perfectly *valid* time values to ms = 
1000 will error out.

I can and have demonstrated a case where NOT allowing msec=1000 is in 
fact detrimental and breaks in current production code (as far as the 
MySQL connection DATETIME values go) for any datetime fraction nearing 
lim{ dt --> 1000ms }. Allow me another demonstration without the DB stuff:

Reproducible test case:
Getting the current ms can be easily written as:
ms := Round(Frac(Now() * 86400)*1000);    // ms is the fraction of the 
seconds multiplied by 1000 and rounded.

Here is a full test-case anyone can run:

procedure timetest;
   var ms   : Integer;
       dtms : TDateTime;
begin
   try
     repeat
       ms := Round(Frac(Now() * 86400)*1000);
       dtms := EncodeTime(00,00,00,ms);
     until false;
   finally
     WriteLn('ms: ',ms);
   end;
end;

It errors out rather quick with the 1000ms culprit problem.

Now as a programmer i have to either TRUNCATE the time, which is not 
great because 12.9ms will become 12ms in stead of 13ms, or add special 
code to test for ms=1000, and what is worse, that code is almost always:

       if (ms=1000) then dtms := EncodeTime(00,00,01,00) else dtms := 
EncodeTime(00,00,00,ms);

I hope it is not lost on any readers how silly that is - I mean what 
ELSE could one possibly end up doing/meaning in the case of 1000ms?.
I will even argue that can be detrimental since I have seen fixes like this:

       if (ms=1000) then ms:=999;
       dtms := EncodeTime(00,00,00,ms);

which drives me to tears of course, but one could almost understand why 
the programmer did that, sacrificing accuracy to get rif of a silly 
exception. It isn't FPC's fault though, that is just a bad programmer.

As programmers our duty is to make our code conform strictly to the 
rules of the underlying systems, and I have been quite a vocal advocate 
for this on other fora - but rules should have clear reasons, not simply 
be rules for the sake of having rules - which comes to my challenge of 
showing a case where it is detrimental.

There was a proposal to compute the specific datetime value for the 
MySQL connector itself in Floating point space, which is a perfect 
solution (and I will be very happy with that too), but does it address 
the full spectrum of plausible cases? Would this need to be fixed in the 
PostGres (etc.) connectors too? Fixing it at the root of date-time 
calculations when the change is indeed sensible, AND barring 
demonstrable "bad" cases (I hasten to add), seems the appropriate thing 
to do.

Lastly...

In other datetime calcs, if you try to add a day that doesn't exist (ex: 
dt := EncodeDate(2021,02,30); ) then the reason it is wrong is clear and 
it should error out, that date does not exist in current calendars. 
Adding 25ms to 02:15:04.975ms simply lands you at 02:15:05.000 - It's 
hard to for me to find a reasonable example of where that will cause 
undue error or a result that is fundamentally wrong.

The only thing I can think that might be a problem is a programmer 
seeing that EncodeTime(02,15,40,1000) yields --> 02:15:41.  It's not 
technically wrong, but would this be unexpected? Would it confuse 
people? (Moreso than throwing an exception?)
If this is the solution we go with, this 1000ms case should probably be 
mentioned clearly in the documentation.


PS: I work with precision time a lot, GPS tracking systems etc, so for 
my use cases this is probably way more of a niggle than everyone else, 
so I concede my opinion doesn't carry all that much weight, but I really 
do not see the harm in fixing this for everyone - unless it can be shown 
to be detrimental, in which case I will happily accept it and appreciate 
being shown it.


Thank you for caring!
Ryan