Recent

Author Topic: Additional opinions on TryEncodeTime and TryEncodeTimeInterval [Swift flavour]  (Read 7957 times)

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
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!?

winni

  • Hero Member
  • *****
  • Posts: 3197
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

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
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.

winni

  • Hero Member
  • *****
  • Posts: 3197
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

Zvoni

  • Hero Member
  • *****
  • Posts: 2317
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
« Last Edit: June 22, 2021, 02:06:07 pm by Zvoni »
One System to rule them all, One Code to find them,
One IDE to bring them all, and to the Framework bind them,
in the Land of Redmond, where the Windows lie
---------------------------------------------------------------------
Code is like a joke: If you have to explain it, it's bad

wp

  • Hero Member
  • *****
  • Posts: 11854
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.

PascalDragon

  • Hero Member
  • *****
  • Posts: 5446
  • Compiler Developer
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 bug report.

Zvoni

  • Hero Member
  • *****
  • Posts: 2317
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);
« Last Edit: June 23, 2021, 10:40:27 am by Zvoni »
One System to rule them all, One Code to find them,
One IDE to bring them all, and to the Framework bind them,
in the Land of Redmond, where the Windows lie
---------------------------------------------------------------------
Code is like a joke: If you have to explain it, it's bad

wp

  • Hero Member
  • *****
  • Posts: 11854
It was by choice and is related to this 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...

Zvoni

  • Hero Member
  • *****
  • Posts: 2317
It was by choice and is related to this 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
One System to rule them all, One Code to find them,
One IDE to bring them all, and to the Framework bind them,
in the Land of Redmond, where the Windows lie
---------------------------------------------------------------------
Code is like a joke: If you have to explain it, it's bad

 

TinyPortal © 2005-2018