Re: [patch, fortran] Fix PR 30814, bounds checking for pack

[Thomas Koenig <tkoenig at alice-dsl dot net>]

On Sat, 2007-07-21 at 00:05 +0200, Tobias Burnus wrote:

Hi Tobias,

> :REVIEWMAIL:
> 
> Thomas Koenig wrote:
> >> OK for trunk if it passes?
> >>     
> > Regression-test was OK.
> >   
> The patch is ok except of two things.
> 
> First, I think one should add to the Fortran documentation of
> -fbounds-check something like:
> "Some checking is only effective if this option is used for the
> compilation of the Fortran main program."

You're right that this should be documented.

I'm still wondering if this is the right approach, though.  I
considered two other options:

a) Pass an additional "bounds_check" argument to the library call.
   Plus:  Can be done on a per-file basis.
   Minus:  Runtime overhead, more compilcated

b) Add another function, with bounds checking.
   Plus: No runtime overhead.
   Minus:  Duplication of code.

I finally setteled on the library flag set by the main program.  Does
everybody agree?

> Secondly, the error message:
> 
> +         /* We come here because of range checking.  */
> +         if (total != ret->dim[0].ubound + 1 - ret->dim[0].lbound)
> +           runtime_error ("Different array shape in return value of"
> +                          " PACK intrinsic");
> 
> 
> I think one should print additionally "total" and the size of ret as
> this helps the user to fix the problem.

We can't do that with runtime_error() at the moment.  We can do it by
making it a printf-style function and call st_printf().

The problem with st_printf is that we can't use it to print 8-byte array
sizes, because it only handles %d and not %ld, so using it in a
straightforward manner would get bogus results for very long arrays.

The obvious solution would be to use vsnprintf() in st_printf(), which
is a C 99 function and not available everywhere...

So, for the time, I'd like to put that off.  I have opened PR 32858 for
this so we don't forget it.

> Additionally, - personal taste - I like "extend", "size" and "length"
> more than "shape" which I find misleading for a rank 1 array.
> 
> Some wording ideas from other compilers (which detect the problem
> already at compile time):

... which we should do as well, if possible.

> NAG f95:
>   Error: a.f90, line 3: Different vector lengths (19 and 30)
> ifort:
>   Error: a.f90, line 3: The shapes of the array expressions do not
> conform.   [NEIGHBRS]
> sunf95:
>   ERROR: The left and right hand sides of this array syntax assignment
> must be conformable arrays.
> openf95:
>   The left and right hand sides of this array syntax assignment must be
> conformable arrays.
> 
> And for the modified program NAG f95 prints at runtime:
>   Rank 1 of array operand has extent 30 instead of 19

Hmm... I like the NAG error versions most.

I'll wait for a day or two for additional comments, then resubmit with
the changed error messages and documentation changes.

	Thomas