Re: [patch, fortran] Implement constant-folding for TRANSFER intrinsic.

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Brooks Moses wrote:
> > Brooks Moses wrote:
> > > +  /* Calculate the size of the source.  */
> > > +  if (source->expr_type == EXPR_ARRAY
> > > +      && gfc_array_size (source, &tmp) == FAILURE)
> > > +    gfc_internal_error ("Failure getting length of a constant array.");
> >
> > I think we're standardizing on gcc_assert even in the frontend.
>
> Even for checking whether a retrieval function (such as gfc_array_size) 
> returns FAILURE?  That seems convoluted, and involves assignment to an 
> extra temporary variable, since it's wrong to put that function call 
> within the assert....

Ouch, I missed that.  Yes, you're right.  The other FEs will call 
gcc_unreachable then, but I don't think that's to be preferred over 
emitting an internal error.

> > > +      if (size)
> >              ^^^^
> > Above you checked size != NULL.  I'd prefer if you omitted that in 
> > both cases.
>
> I'm not sure I follow how this check can legitimately be omitted in 
> either case, without producing wrong results.  The SIZE argument is 
> optional, and thus can legitimately be null.  Also, before this we 
> checked that MOLD is an array _or_ size != NULL; either one of those 
> situations will cause the result to be an array, and so this check isn't 
> redundant.
>
> Or do you merely mean that the "NULL" part should be omitted, and the 
> earlier version of it should be written as "!size" rather than as "size 
> != NULL"?  That makes sense.

The latter, sorry for the confusion.

> > > +       result_length = (size_t)mpz_get_ui (size->value.integer);
> > > +      else
> > > +       {
> > > +         result_length = source_size / result_elt_size;
> > > +         if (result_length * result_elt_size < source_size)
> > > +           result_length += 1;
> > > +       }
> >
> > result_length = (source_size - 1) / result_elt_size + 1;
> >
> > Don't know if that's harder to understand.
>
> I find it a bit harder to understand, personally, but I'm not very 
> familiar with this sort of integer-division trick.

My suggestion may also break with zero source_sizes, so I think we 
should stay with yours.

> > What about targets with different character sets?
>
> They are currently hypothetical; even the S390 is only currently 
> supported in an ASCII configuration as far as I can tell.
>
> It's a valid concern beyond that, but I'm not sure what to do with it.

I thought the S390 used EBCDIC unconditionally, if that's not the case, 
I agree that it's not a real problem.

> My inclination would be to do the cosmetic and small fixes to this and 
> go ahead and commit it and then work on the alternate-character-set 
> hypotheticality and the derived-type spacing stuff in follow-up patches, 
> since AFAIK this doesn't cause anything to break that worked 
> previously.  But that's not a strong inclination, and I'll let Paul and 
> FX make the call on that.  :)

I think we should assert that we're not dealing with derived types, 
since otherwise we would leave the user with a potentially hard-to-find 
error.  Apart from that, I agree with this course of action.

- Tobi