Re: [Patch, Fortran] PR fortran/37099: Wrong simplification of constant character arrays

[Daniel Kraft <d at domob dot eu>]

Paul Richard Thomas wrote:
> > missing cloog).  Ok to commit?  If it is,
> > should I also backport to 4.3?
>
> OK for both, with the usual delay for 4.3, with one proviso below.
>
> > Just a remark, I don't quite like the current test's name
> > (string_compare_1.f90), but it is the only thing I came up with.  Do you
> > have a better idea?  It's a reduced version of the test attached to the
> > PR.  Do you think I should include those original tests, too?  (Both
> > succeed with my fix, of course.)
>
> I have been caught more than once with a reduced testcase revealing a
> different problem to the original.  Finding the PR reopened because it
> wasn'tactually fixed is an unpleasant surprise.  I would include the
> original test.

I'll do so very happily as string_compare_2/3.f90.  I'm just wondering 
if we don't oppose "too many" tests which might be similar.

> > +
> > +                     gcc_assert (p->ref->next);
> > +                     gcc_assert (!p->ref->next->next);
> > +                     gcc_assert (p->ref->next->type == REF_SUBSTRING);
> > +
> > +                     if (p->value.constructor)
> > +                       {
> > +                         const gfc_expr* first =
> > p->value.constructor->expr;
> > +                         gcc_assert (first->expr_type == EXPR_CONSTANT);
> > +                         gcc_assert (first->ts.type == BT_CHARACTER);
>
> Is there any reason to go so heavy on the gcc_asserts?  I don't have
> anything against it but ask myself if you are anticipating problems
> here?

Not really, I'm just used to doing lots of assertions (in my own codes, 
too).  I believe its better to have those in, as one of them failing is 
probably much, much easier to debug than some spurious failure or wrong 
code.  I'm not sure if gcc releases disable assertions by default or 
don't do so and if we have to be worried about performance in some 
cases, though.

But if that's ok for you, I'll leave them in here.  But feel free to 
comment on those in my patches in the future :)

Thanks for the review!

Daniel