[PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Simon Wright]

The description of the second Value function (returning Duration) (ARM 9.6.1(87) 
doesn't place any limitation on the Elapsed_Time parameter's value, beyond 
"Constraint_Error is raised if the string is not formatted as described for Image, or 
the function cannot interpret the given string as a Duration value".

It would seem reasonable that Value and Image should be consistent, in that any 
string produced by Image should be accepted by Value. Since Image must produce
a two-digit representation of the Hours, there's an implication that its 
Elapsed_Time parameter should be less than 100.0 hours (the ARM merely says
that in that case the result is implementation-defined).

The current implementation of Value raises Constraint_Error if the Elapsed_Time
parameter is greater than or equal to 24 hours.

This patch removes the restriction, so that the Elapsed_Time parameter must only
be less than 100.0 hours.

gcc/ada/Changelog:

  2023-10-15 Simon Wright <simon@pushface.org>

  PR ada/111813

  * gcc/ada/libgnat/a-calfor.adb (Value (2)): Allow values of parameter
      Elapsed_Time greater than or equal to 24 hours, by doing the
      hour calculations in Natural rather than Hour_Number (0 .. 23).
      Calculate the result directly rather than by using Seconds_Of
      (whose Hour parameter is of type Hour_Number).

      If an exception occurs of type Constraint_Error, re-raise it
      rather than raising a new CE.

gcc/testsuite/Changelog:

  2023-10-15 Simon Wright <simon@pushface.org>

  PR ada/111813

  * gcc/testsuite/gnat.dg/calendar_format_value.adb: New test.

---
 gcc/ada/libgnat/a-calfor.adb                  | 11 +++++---
 .../gnat.dg/calendar_format_value.adb         | 26 +++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gnat.dg/calendar_format_value.adb

diff --git a/gcc/ada/libgnat/a-calfor.adb b/gcc/ada/libgnat/a-calfor.adb
index 18f4e7388df..493728b490e 100644
--- a/gcc/ada/libgnat/a-calfor.adb
+++ b/gcc/ada/libgnat/a-calfor.adb
@@ -777,7 +777,7 @@ package body Ada.Calendar.Formatting is
 
    function Value (Elapsed_Time : String) return Duration is
       D          : String (1 .. 11);
-      Hour       : Hour_Number;
+      Hour       : Natural;
       Minute     : Minute_Number;
       Second     : Second_Number;
       Sub_Second : Second_Duration := 0.0;
@@ -817,7 +817,7 @@ package body Ada.Calendar.Formatting is
 
       --  Value extraction
 
-      Hour   := Hour_Number   (Hour_Number'Value   (D (1 .. 2)));
+      Hour   := Natural       (Natural'Value       (D (1 .. 2)));
       Minute := Minute_Number (Minute_Number'Value (D (4 .. 5)));
       Second := Second_Number (Second_Number'Value (D (7 .. 8)));
 
@@ -837,9 +837,14 @@ package body Ada.Calendar.Formatting is
          raise Constraint_Error;
       end if;
 
-      return Seconds_Of (Hour, Minute, Second, Sub_Second);
+      return Duration (Hour * 3600)
+        + Duration (Minute * 60)
+        + Duration (Second)
+        + Sub_Second;
 
    exception
+      --  CE is mandated, but preserve trace if CE already.
+      when Constraint_Error => raise;
       when others => raise Constraint_Error;
    end Value;
 
diff --git a/gcc/testsuite/gnat.dg/calendar_format_value.adb b/gcc/testsuite/gnat.dg/calendar_format_value.adb
new file mode 100644
index 00000000000..e98e496fd3b
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/calendar_format_value.adb
@@ -0,0 +1,26 @@
+-- { dg-do run }
+-- { dg-options "-O2" }
+
+with Ada.Calendar.Formatting;
+
+procedure Calendar_Format_Value is
+   Limit : constant Duration
+     := 99 * 3600.0 + 59 * 60.0 + 59.0;
+begin
+   declare
+      Image : constant String := Ada.Calendar.Formatting .Image (Limit);
+      Image_Error : exception;
+   begin
+      if Image /= "99:59:59" then
+         raise Image_Error with "image: " & Image;
+      end if;
+      declare
+         Value : constant Duration := Ada.Calendar.Formatting.Value (Image);
+         Value_Error : exception;
+      begin
+         if Value /= Limit then
+            raise Value_Error with "duration: " & Value'Image;
+         end if;
+      end;
+   end;
+end Calendar_Format_Value;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Simon Wright]

Pierre-Marie, I’ve CC’d you hoping you’re an appropriate person to ping on this one.
If not, who would be for this sort of change?

I should have said, tested by
- add test case, make -C gcc check-gnat: error reported
- make -C gcc gnatlib_and_tools; make install
- make -C gcc check-gnat: no error reported

FSF copyright assignment RT:1016382

—S

> On 16 Oct 2023, at 14:32, Simon Wright <simon@pushface.org> wrote:
> 
> The description of the second Value function (returning Duration) (ARM 9.6.1(87) 
> doesn't place any limitation on the Elapsed_Time parameter's value, beyond 
> "Constraint_Error is raised if the string is not formatted as described for Image, or 
> the function cannot interpret the given string as a Duration value".
> 
> It would seem reasonable that Value and Image should be consistent, in that any 
> string produced by Image should be accepted by Value. Since Image must produce
> a two-digit representation of the Hours, there's an implication that its 
> Elapsed_Time parameter should be less than 100.0 hours (the ARM merely says
> that in that case the result is implementation-defined).
> 
> The current implementation of Value raises Constraint_Error if the Elapsed_Time
> parameter is greater than or equal to 24 hours.
> 
> This patch removes the restriction, so that the Elapsed_Time parameter must only
> be less than 100.0 hours.
> 
> gcc/ada/Changelog:
> 
>  2023-10-15 Simon Wright <simon@pushface.org>
> 
>  PR ada/111813
> 
>  * gcc/ada/libgnat/a-calfor.adb (Value (2)): Allow values of parameter
>      Elapsed_Time greater than or equal to 24 hours, by doing the
>      hour calculations in Natural rather than Hour_Number (0 .. 23).
>      Calculate the result directly rather than by using Seconds_Of
>      (whose Hour parameter is of type Hour_Number).
> 
>      If an exception occurs of type Constraint_Error, re-raise it
>      rather than raising a new CE.
> 
> gcc/testsuite/Changelog:
> 
>  2023-10-15 Simon Wright <simon@pushface.org>
> 
>  PR ada/111813
> 
>  * gcc/testsuite/gnat.dg/calendar_format_value.adb: New test.
> 
> ---
> gcc/ada/libgnat/a-calfor.adb                  | 11 +++++---
> .../gnat.dg/calendar_format_value.adb         | 26 +++++++++++++++++++
> 2 files changed, 34 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gnat.dg/calendar_format_value.adb
> 
> diff --git a/gcc/ada/libgnat/a-calfor.adb b/gcc/ada/libgnat/a-calfor.adb
> index 18f4e7388df..493728b490e 100644
> --- a/gcc/ada/libgnat/a-calfor.adb
> +++ b/gcc/ada/libgnat/a-calfor.adb
> @@ -777,7 +777,7 @@ package body Ada.Calendar.Formatting is
> 
>    function Value (Elapsed_Time : String) return Duration is
>       D          : String (1 .. 11);
> -      Hour       : Hour_Number;
> +      Hour       : Natural;
>       Minute     : Minute_Number;
>       Second     : Second_Number;
>       Sub_Second : Second_Duration := 0.0;
> @@ -817,7 +817,7 @@ package body Ada.Calendar.Formatting is
> 
>       --  Value extraction
> 
> -      Hour   := Hour_Number   (Hour_Number'Value   (D (1 .. 2)));
> +      Hour   := Natural       (Natural'Value       (D (1 .. 2)));
>       Minute := Minute_Number (Minute_Number'Value (D (4 .. 5)));
>       Second := Second_Number (Second_Number'Value (D (7 .. 8)));
> 
> @@ -837,9 +837,14 @@ package body Ada.Calendar.Formatting is
>          raise Constraint_Error;
>       end if;
> 
> -      return Seconds_Of (Hour, Minute, Second, Sub_Second);
> +      return Duration (Hour * 3600)
> +        + Duration (Minute * 60)
> +        + Duration (Second)
> +        + Sub_Second;
> 
>    exception
> +      --  CE is mandated, but preserve trace if CE already.
> +      when Constraint_Error => raise;
>       when others => raise Constraint_Error;
>    end Value;
> 
> diff --git a/gcc/testsuite/gnat.dg/calendar_format_value.adb b/gcc/testsuite/gnat.dg/calendar_format_value.adb
> new file mode 100644
> index 00000000000..e98e496fd3b
> --- /dev/null
> +++ b/gcc/testsuite/gnat.dg/calendar_format_value.adb
> @@ -0,0 +1,26 @@
> +-- { dg-do run }
> +-- { dg-options "-O2" }
> +
> +with Ada.Calendar.Formatting;
> +
> +procedure Calendar_Format_Value is
> +   Limit : constant Duration
> +     := 99 * 3600.0 + 59 * 60.0 + 59.0;
> +begin
> +   declare
> +      Image : constant String := Ada.Calendar.Formatting .Image (Limit);
> +      Image_Error : exception;
> +   begin
> +      if Image /= "99:59:59" then
> +         raise Image_Error with "image: " & Image;
> +      end if;
> +      declare
> +         Value : constant Duration := Ada.Calendar.Formatting.Value (Image);
> +         Value_Error : exception;
> +      begin
> +         if Value /= Limit then
> +            raise Value_Error with "duration: " & Value'Image;
> +         end if;
> +      end;
> +   end;
> +end Calendar_Format_Value;
> -- 
> 2.39.3 (Apple Git-145)
> 

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Arnaud Charlet]

Hi Simon,

> Pierre-Marie, I’ve CC’d you hoping you’re an appropriate person to ping on this one.
> If not, who would be for this sort of change?
> 
> I should have said, tested by
> - add test case, make -C gcc check-gnat: error reported
> - make -C gcc gnatlib_and_tools; make install
> - make -C gcc check-gnat: no error reported
> 
> FSF copyright assignment RT:1016382

Your message hasn't been forgotten. We're currently reviewing it at AdaCore,
this is taking some time.

I'll get back to you when I have some feedback.

Regards,

Arno

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Arnaud Charlet]

This change is OK, thank you.

> The description of the second Value function (returning Duration) (ARM 9.6.1(87) 
> doesn't place any limitation on the Elapsed_Time parameter's value, beyond 
> "Constraint_Error is raised if the string is not formatted as described for Image, or 
> the function cannot interpret the given string as a Duration value".
> 
> It would seem reasonable that Value and Image should be consistent, in that any 
> string produced by Image should be accepted by Value. Since Image must produce
> a two-digit representation of the Hours, there's an implication that its 
> Elapsed_Time parameter should be less than 100.0 hours (the ARM merely says
> that in that case the result is implementation-defined).
> 
> The current implementation of Value raises Constraint_Error if the Elapsed_Time
> parameter is greater than or equal to 24 hours.
> 
> This patch removes the restriction, so that the Elapsed_Time parameter must only
> be less than 100.0 hours.
> 
> gcc/ada/Changelog:
> 
>   2023-10-15 Simon Wright <simon@pushface.org>
> 
>   PR ada/111813
> 
>   * gcc/ada/libgnat/a-calfor.adb (Value (2)): Allow values of parameter
>       Elapsed_Time greater than or equal to 24 hours, by doing the
>       hour calculations in Natural rather than Hour_Number (0 .. 23).
>       Calculate the result directly rather than by using Seconds_Of
>       (whose Hour parameter is of type Hour_Number).
> 
>       If an exception occurs of type Constraint_Error, re-raise it
>       rather than raising a new CE.
> 
> gcc/testsuite/Changelog:
> 
>   2023-10-15 Simon Wright <simon@pushface.org>
> 
>   PR ada/111813
> 
>   * gcc/testsuite/gnat.dg/calendar_format_value.adb: New test.

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Simon Wright]

On 24 Oct 2023, at 10:49, Arnaud Charlet <charlet@adacore.com> wrote:
> 
> This change is OK, thank you.

Can it be committed, then, please?

>> The description of the second Value function (returning Duration) (ARM 9.6.1(87) 
>> doesn't place any limitation on the Elapsed_Time parameter's value, beyond 
>> "Constraint_Error is raised if the string is not formatted as described for Image, or 
>> the function cannot interpret the given string as a Duration value".
>> 
>> It would seem reasonable that Value and Image should be consistent, in that any 
>> string produced by Image should be accepted by Value. Since Image must produce
>> a two-digit representation of the Hours, there's an implication that its 
>> Elapsed_Time parameter should be less than 100.0 hours (the ARM merely says
>> that in that case the result is implementation-defined).
>> 
>> The current implementation of Value raises Constraint_Error if the Elapsed_Time
>> parameter is greater than or equal to 24 hours.
>> 
>> This patch removes the restriction, so that the Elapsed_Time parameter must only
>> be less than 100.0 hours.
>> 
>> gcc/ada/Changelog:
>> 
>>  2023-10-15 Simon Wright <simon@pushface.org>
>> 
>>  PR ada/111813
>> 
>>  * gcc/ada/libgnat/a-calfor.adb (Value (2)): Allow values of parameter
>>      Elapsed_Time greater than or equal to 24 hours, by doing the
>>      hour calculations in Natural rather than Hour_Number (0 .. 23).
>>      Calculate the result directly rather than by using Seconds_Of
>>      (whose Hour parameter is of type Hour_Number).
>> 
>>      If an exception occurs of type Constraint_Error, re-raise it
>>      rather than raising a new CE.
>> 
>> gcc/testsuite/Changelog:
>> 
>>  2023-10-15 Simon Wright <simon@pushface.org>
>> 
>>  PR ada/111813
>> 
>>  * gcc/testsuite/gnat.dg/calendar_format_value.adb: New test.

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Arnaud Charlet]

Marc, can you please take care of it when you get a chance?

On Thu, Nov 09, 2023 at 11:22:21AM +0000, Simon Wright wrote:
> On 24 Oct 2023, at 10:49, Arnaud Charlet <charlet@adacore.com> wrote:
> > 
> > This change is OK, thank you.
> 
> Can it be committed, then, please?
> 
> >> The description of the second Value function (returning Duration) (ARM 9.6.1(87) 
> >> doesn't place any limitation on the Elapsed_Time parameter's value, beyond 
> >> "Constraint_Error is raised if the string is not formatted as described for Image, or 
> >> the function cannot interpret the given string as a Duration value".
> >> 
> >> It would seem reasonable that Value and Image should be consistent, in that any 
> >> string produced by Image should be accepted by Value. Since Image must produce
> >> a two-digit representation of the Hours, there's an implication that its 
> >> Elapsed_Time parameter should be less than 100.0 hours (the ARM merely says
> >> that in that case the result is implementation-defined).
> >> 
> >> The current implementation of Value raises Constraint_Error if the Elapsed_Time
> >> parameter is greater than or equal to 24 hours.
> >> 
> >> This patch removes the restriction, so that the Elapsed_Time parameter must only
> >> be less than 100.0 hours.
> >> 
> >> gcc/ada/Changelog:
> >> 
> >>  2023-10-15 Simon Wright <simon@pushface.org>
> >> 
> >>  PR ada/111813
> >> 
> >>  * gcc/ada/libgnat/a-calfor.adb (Value (2)): Allow values of parameter
> >>      Elapsed_Time greater than or equal to 24 hours, by doing the
> >>      hour calculations in Natural rather than Hour_Number (0 .. 23).
> >>      Calculate the result directly rather than by using Seconds_Of
> >>      (whose Hour parameter is of type Hour_Number).
> >> 
> >>      If an exception occurs of type Constraint_Error, re-raise it
> >>      rather than raising a new CE.
> >> 
> >> gcc/testsuite/Changelog:
> >> 
> >>  2023-10-15 Simon Wright <simon@pushface.org>
> >> 
> >>  PR ada/111813
> >> 
> >>  * gcc/testsuite/gnat.dg/calendar_format_value.adb: New test.

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Marc Poulhiès]

Arnaud Charlet <charlet@adacore.com> writes:

> Marc, can you please take care of it when you get a chance?

I'll push the change as soon as the tests are finished.

Marc

Re: [PATCH] Fix PR ada/111813 (Inconsistent limit in Ada.Calendar.Formatting)

[Marc Poulhiès]

Marc Poulhiès <poulhies@adacore.com> writes:

> Arnaud Charlet <charlet@adacore.com> writes:
>
>> Marc, can you please take care of it when you get a chance?
>
> I'll push the change as soon as the tests are finished.

Pushed as r14-5282.

Marc