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

[Daniel Kraft]

Daniel Kraft wrote:
> 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.

Committed as 139997 to trunk with the changes mentioned below.  I'll 
wait until next week and do a 4.3 backport then.  I'll close the PR if 
that's done, too.

Daniel

>>
>>> 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
> 


-- 
Done:     Arc-Bar-Cav-Sam-Val-Wiz, Dwa-Elf-Gno-Hum-Orc, Law-Neu-Cha, Fem-Mal
To go:    Hea-Kni-Mon-Pri-Ran-Rog-Tou