Lazarus

Free Pascal => FPC development => Topic started by: lagprogramming on June 21, 2021, 05:03:11 pm

Title: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: lagprogramming on June 21, 2021, 05:03:11 pm
rtl/objpas/sysutils/dati.inc.TryEncodeTime contains the following source code:
Code: Pascal  [Select][+][-]
  1. function TryEncodeTime(Hour, Min, Sec, MSec:word; Out Time : TDateTime) : boolean;
  2. begin
  3.   Result:=(Hour<24) and (Min<60) and (Sec<60) and (MSec<1000);
  4.   If Result then
  5.     Time:=TDateTime(cardinal(Hour)*3600000+cardinal(Min)*60000+cardinal(Sec)*1000+MSec)/MSecsPerDay;
  6. end;

packages/rtl-objpas/src/inc/dateutil.inc.TryEncodeTimeInterval contains the following source code:
Code: Pascal  [Select][+][-]
  1. function TryEncodeTimeInterval(Hour, Min, Sec, MSec: word; out Time: TDateTime): boolean;
  2. begin
  3.  Result:= (Min<60) and (Sec<60) and (MSec<=1000);
  4.  If Result then
  5.    Time:=TDateTime(cardinal(Hour)*3600000+cardinal(Min)*60000+cardinal(Sec)*1000+MSec)/MSecsPerDay;
  6. end;
  7.  

   This inconsistency between result assignments make me feel that there might be a bug in one of these functions. Doesn't look strange to you, too!?
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: winni on June 21, 2021, 05:48:35 pm
Hi!

No, there is no inconsistency.

At computing the interval there might be the possibility that hour is >= 24.
As there is no day parameter the computing will automatic wrap it into one or more days.

For the encoding of the time this it not legal.
So everything is okay.

Winni
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: lagprogramming on June 22, 2021, 12:44:59 pm
Knowing that 1 second equals 1000miliseconds, 1minute equals 60seconds and 1hour equals 60minutes, why is 1000 a valid value for MSec but 60 is not a valid value for Sec and Min parameters?!
A while ago, TryEncodeTimeInterval used "(MSec<1000);" but the source code has been changed to "(MSec<=1000);".
TryEncodeTimeInterval(0,0,59,1000,Time) returns true while TryEncodeTimeInterval(0,0,60,0,Time) returns false.
If you're sure that there is no problem, so be it. It's just that, to me, it's not too obvious what the result of the function really means.
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: winni on June 22, 2021, 01:04:31 pm
Hi!

Yes Mister Millisecond - you are right!

But I am not happy with the restrictions for TryEncodeTimeInterval.

If you are computing the Interval on the base of hours/minutes/seconds/ms you might get an underflow for one or more of the values. But that does not matter: The computing of the TDateTime will resolve that automatic. This restriction only complicates the computing.

But I think this function only exists due to Delphi-compatibility-reasons.

It is much easier to do

Code: Pascal  [Select][+][-]
  1. DateTime_Interval := DateTime_End - DateTime_Start;
  2.  

Winni
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: Zvoni on June 22, 2021, 02:04:24 pm
Knowing that 1 second equals 1000miliseconds, 1minute equals 60seconds and 1hour equals 60minutes, why is 1000 a valid value for MSec but 60 is not a valid value for Sec and Min parameters?!
A while ago, TryEncodeTimeInterval used "(MSec<1000);" but the source code has been changed to "(MSec<=1000);".
TryEncodeTimeInterval(0,0,59,1000,Time) returns true while TryEncodeTimeInterval(0,0,60,0,Time) returns false.
If you're sure that there is no problem, so be it. It's just that, to me, it's not too obvious what the result of the function really means.

Because there is no value "60" for a second (or minute) --> It doesn't exist!

The first minute of the hour is the minute with value "0" (same for seconds)
the "sixtieth" second/minute is the one with value 59
The same for milliseconds: the 1000th millisecond is actually the first millisecond of the next second (equaling 0),
or the other way round: the 0th Millisecond is actually the first

Another clue for your millisecond-"problem" --> Look at all the Time-Format-Strings "hh:mm:ss:zzz" --> only three digits for Millisecond

The Day starts at "00:00:00:000" --> "zero hours:zero minutes:zero seconds:zero milliseconds"

Having said that:
MSec<=1000 is plain wrong
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: wp on June 22, 2021, 04:28:08 pm
A while ago, TryEncodeTimeInterval used "(MSec<1000);" but the source code has been changed to "(MSec<=1000);".
This was introduced intentionally by Michael in r48380 on Jan 24 ("Allow milliseconds up to 1000 milliseconds"), no idea why he did this. In my opinion this is wrong and you should file a bugreport.
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: PascalDragon on June 23, 2021, 09:06:02 am
A while ago, TryEncodeTimeInterval used "(MSec<1000);" but the source code has been changed to "(MSec<=1000);".
This was introduced intentionally by Michael in r48380 on Jan 24 ("Allow milliseconds up to 1000 milliseconds"), no idea why he did this. In my opinion this is wrong and you should file a bugreport.

It was by choice and is related to this (https://bugs.freepascal.org/view.php?id=37849) bug report.
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: Zvoni on June 23, 2021, 09:27:50 am
Still looks wrong (Sorry, personal opinion)

Yes, i'm aware a simple "If Round(MSec)=1000 Then MSec:=0;" doesn't solve it, since you would have to walk the whole chain (seconds, minutes) upwards checking if it carries over to the next time-unit.

Basically, you would have to introduce a fallthrough-check before "Result:= (Min<60) and (Sec<60) and (MSec<1000);" (=-sign removed) in TryEncodeTimeInterval, adjusting each time-unit depending if it reaches its max-value

This is Aircode!
Code: Pascal  [Select][+][-]
  1. If MSec=1000 Then Begin  
  2. //Or '>='? In that case you obviously have to change the Inc-Call to include the 'div'-result --> Inc(Sec, MSec div 1000);
  3. //and the adjusted value would have to include the modulo MSec:=MSec mod 1000;
  4.   MSec:=0;
  5.   Inc(Sec);
  6. End;
  7. If Sec=60 Then Begin  
  8. //Or '>='?  In that case you obviously have to change the Inc-Call to include the 'div'-result --> Inc(Min, Sec div 60);
  9. //and the adjusted value would have to include the modulo Sec:=Sec mod 60;
  10.   Sec:=0;
  11.   Inc(Min);
  12. End;
  13. If Min=60 Then Begin  
  14. //Or '>='? In that case you obviously have to change the Inc-Call to include the 'div'-result --> Inc(Hour, Min div 60);
  15. //and the adjusted value would have to include the modulo Min:=Min mod 60;
  16.    Min:=0;
  17.    Inc(Hour);
  18. End;
  19. Result:=(Min<60) and (Sec<60) and (MSec<1000);
  20.  

EDIT: Looking at my Aircode it could be reworked as described in the comments to allow something like
ABoolean:=TryEncodeTimeInterval(0,0,143,12345,ADateTime);
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: wp on June 23, 2021, 09:48:08 am
It was by choice and is related to this (https://bugs.freepascal.org/view.php?id=37849) bug report.
Strange decision. A separate function should have been provided for this particular case rather than polluting a general purpose function. Waiting for next bugreport when somebody finds a case that minutes or seconds round to 60...
Title: Re: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]
Post by: Zvoni on June 23, 2021, 11:02:08 am
It was by choice and is related to this (https://bugs.freepascal.org/view.php?id=37849) bug report.
Strange decision. A separate function should have been provided for this particular case rather than polluting a general purpose function. Waiting for next bugreport when somebody finds a case that minutes or seconds round to 60...

This could only happen, if you have fractional values for minutes and/or seconds and/or hours before calling the function, in which case you are forced to "cast" them to Type Word (notice the quotes) using whatever method to get from a Float to a Word (Trunc, Round, whatever).
But you're right: it could result in 60 seconds, minutes whatever....

See my edit above and my aircode
TinyPortal © 2005-2018