public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Harald Anlauf <anlauf@gmx.de>
To: Paul Richard Thomas <paul.richard.thomas@gmail.com>
Cc: "fortran@gcc.gnu.org" <fortran@gcc.gnu.org>,
	gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [Patch, fortran] PR49213 - [OOP] gfortran rejects structure constructor expression
Date: Tue, 27 Jun 2023 21:27:06 +0200	[thread overview]
Message-ID: <152f9e8b-6998-e505-7875-14dc14268382@gmx.de> (raw)
In-Reply-To: <CAGkQGiKrT2m4y+y_83NBZ_4+3E=vr+pPRtxVe3hDgkOn9RU5nw@mail.gmail.com>

Hi Paul,

this is much better now.

I have only a minor comment left: in the calculation of the
size of a character string you are using an intermediate
gfc_array_index_type, whereas I have learned to use
gfc_charlen_type_node now, which seems like the natural
type here.

OK for trunk, and thanks for your patience!

Harald


On 6/27/23 12:30, Paul Richard Thomas via Gcc-patches wrote:
> Hi Harald,
>
> Let's try again :-)
>
> OK for trunk?
>
> Regards
>
> Paul
>
> Fortran: Enable class expressions in structure constructors [PR49213]
>
> 2023-06-27  Paul Thomas  <pault@gcc.gnu.org>
>
> gcc/fortran
> PR fortran/49213
> * expr.cc (gfc_is_ptr_fcn): Remove reference to class_pointer.
> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
> associate names with pointer function targets to be used in
> variable definition context.
> * trans-decl.cc (get_symbol_decl): Remove extraneous line.
> * trans-expr.cc (alloc_scalar_allocatable_subcomponent): Obtain
> size of intrinsic and character expressions.
> (gfc_trans_subcomponent_assign): Expand assignment to class
> components to include intrinsic and character expressions.
>
> gcc/testsuite/
> PR fortran/49213
> * gfortran.dg/pr49213.f90 : New test
>
> On Sat, 24 Jun 2023 at 20:50, Harald Anlauf <anlauf@gmx.de> wrote:
>>
>> Hi Paul!
>>
>> On 6/24/23 15:18, Paul Richard Thomas via Gcc-patches wrote:
>>> I have included the adjustment to 'gfc_is_ptr_fcn' and eliminating the
>>> extra blank line, introduced by my last patch. I played safe and went
>>> exclusively for class functions with attr.class_pointer set on the
>>> grounds that these have had all the accoutrements checked and built
>>> (ie. class_ok). I am still not sure if this is necessary or not.
>>
>> maybe it is my fault, but I find the version in the patch confusing:
>>
>> @@ -816,7 +816,7 @@ bool
>>    gfc_is_ptr_fcn (gfc_expr *e)
>>    {
>>      return e != NULL && e->expr_type == EXPR_FUNCTION
>> -             && (gfc_expr_attr (e).pointer
>> +             && ((e->ts.type != BT_CLASS && gfc_expr_attr (e).pointer)
>>                     || (e->ts.type == BT_CLASS
>>                         && CLASS_DATA (e)->attr.class_pointer));
>>    }
>>
>> The caller 'gfc_is_ptr_fcn' has e->expr_type == EXPR_FUNCTION, so
>> gfc_expr_attr (e) boils down to:
>>
>>         if (e->value.function.esym && e->value.function.esym->result)
>>          {
>>            gfc_symbol *sym = e->value.function.esym->result;
>>            attr = sym->attr;
>>            if (sym->ts.type == BT_CLASS && sym->attr.class_ok)
>>              {
>>                attr.dimension = CLASS_DATA (sym)->attr.dimension;
>>                attr.pointer = CLASS_DATA (sym)->attr.class_pointer;
>>                attr.allocatable = CLASS_DATA (sym)->attr.allocatable;
>>              }
>>          }
>> ...
>>         else if (e->symtree)
>>          attr = gfc_variable_attr (e, NULL);
>>
>> So I thought this should already do what you want if you do
>>
>> gfc_is_ptr_fcn (gfc_expr *e)
>> {
>>     return e != NULL && e->expr_type == EXPR_FUNCTION && gfc_expr_attr
>> (e).pointer;
>> }
>>
>> or what am I missing?  The additional checks in gfc_expr_attr are
>> there to avoid ICEs in case CLASS_DATA (sym) has issues, and we all
>> know Gerhard who showed that he is an expert in exploiting this.
>>
>> To sum up, I'd prefer to use the safer form if it works.  If it
>> doesn't, I would expect a latent issue.
>>
>> The rest of the code looked good to me, but I was suspicious about
>> the handling of CHARACTER.
>>
>> Nasty as I am, I modified the testcase to use character(kind=4)
>> instead of kind=1 (see attached).  This either fails here (stop 10),
>> or if I activate the marked line
>>
>> !    cont = tContainer('hello!')       ! ### ICE! ###
>>
>> I get an ICE.
>>
>> Can you have another look?
>>
>> Thanks,
>> Harald
>>
>>>
>>
>>> OK for trunk?
>>>
>>> Paul
>>>
>>> Fortran: Enable class expressions in structure constructors [PR49213]
>>>
>>> 2023-06-24  Paul Thomas  <pault@gcc.gnu.org>
>>>
>>> gcc/fortran
>>> PR fortran/49213
>>> * expr.cc (gfc_is_ptr_fcn): Guard pointer attribute to exclude
>>> class expressions.
>>> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
>>> associate names with pointer function targets to be used in
>>> variable definition context.
>>> * trans-decl.cc (get_symbol_decl): Remove extraneous line.
>>> * trans-expr.cc (alloc_scalar_allocatable_subcomponent): Obtain
>>> size of intrinsic and character expressions.
>>> (gfc_trans_subcomponent_assign): Expand assignment to class
>>> components to include intrinsic and character expressions.
>>>
>>> gcc/testsuite/
>>> PR fortran/49213
>>> * gfortran.dg/pr49213.f90 : New test
>
>
>


WARNING: multiple messages have this Message-ID
From: Harald Anlauf <anlauf@gmx.de>
To: gcc-patches@gcc.gnu.org
Cc: fortran@gcc.gnu.org
Subject: Re: [Patch, fortran] PR49213 - [OOP] gfortran rejects structure constructor expression
Date: Tue, 27 Jun 2023 21:27:06 +0200	[thread overview]
Message-ID: <152f9e8b-6998-e505-7875-14dc14268382@gmx.de> (raw)
Message-ID: <20230627192706.oUE9u0hQmTTxdB0UwzHyTTM1XVKO13xFaaqV_WnZuw0@z> (raw)
In-Reply-To: <CAGkQGiKrT2m4y+y_83NBZ_4+3E=vr+pPRtxVe3hDgkOn9RU5nw@mail.gmail.com>

Hi Paul,

this is much better now.

I have only a minor comment left: in the calculation of the
size of a character string you are using an intermediate
gfc_array_index_type, whereas I have learned to use
gfc_charlen_type_node now, which seems like the natural
type here.

OK for trunk, and thanks for your patience!

Harald


On 6/27/23 12:30, Paul Richard Thomas via Gcc-patches wrote:
> Hi Harald,
> 
> Let's try again :-)
> 
> OK for trunk?
> 
> Regards
> 
> Paul
> 
> Fortran: Enable class expressions in structure constructors [PR49213]
> 
> 2023-06-27  Paul Thomas  <pault@gcc.gnu.org>
> 
> gcc/fortran
> PR fortran/49213
> * expr.cc (gfc_is_ptr_fcn): Remove reference to class_pointer.
> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
> associate names with pointer function targets to be used in
> variable definition context.
> * trans-decl.cc (get_symbol_decl): Remove extraneous line.
> * trans-expr.cc (alloc_scalar_allocatable_subcomponent): Obtain
> size of intrinsic and character expressions.
> (gfc_trans_subcomponent_assign): Expand assignment to class
> components to include intrinsic and character expressions.
> 
> gcc/testsuite/
> PR fortran/49213
> * gfortran.dg/pr49213.f90 : New test
> 
> On Sat, 24 Jun 2023 at 20:50, Harald Anlauf <anlauf@gmx.de> wrote:
>>
>> Hi Paul!
>>
>> On 6/24/23 15:18, Paul Richard Thomas via Gcc-patches wrote:
>>> I have included the adjustment to 'gfc_is_ptr_fcn' and eliminating the
>>> extra blank line, introduced by my last patch. I played safe and went
>>> exclusively for class functions with attr.class_pointer set on the
>>> grounds that these have had all the accoutrements checked and built
>>> (ie. class_ok). I am still not sure if this is necessary or not.
>>
>> maybe it is my fault, but I find the version in the patch confusing:
>>
>> @@ -816,7 +816,7 @@ bool
>>    gfc_is_ptr_fcn (gfc_expr *e)
>>    {
>>      return e != NULL && e->expr_type == EXPR_FUNCTION
>> -             && (gfc_expr_attr (e).pointer
>> +             && ((e->ts.type != BT_CLASS && gfc_expr_attr (e).pointer)
>>                     || (e->ts.type == BT_CLASS
>>                         && CLASS_DATA (e)->attr.class_pointer));
>>    }
>>
>> The caller 'gfc_is_ptr_fcn' has e->expr_type == EXPR_FUNCTION, so
>> gfc_expr_attr (e) boils down to:
>>
>>         if (e->value.function.esym && e->value.function.esym->result)
>>          {
>>            gfc_symbol *sym = e->value.function.esym->result;
>>            attr = sym->attr;
>>            if (sym->ts.type == BT_CLASS && sym->attr.class_ok)
>>              {
>>                attr.dimension = CLASS_DATA (sym)->attr.dimension;
>>                attr.pointer = CLASS_DATA (sym)->attr.class_pointer;
>>                attr.allocatable = CLASS_DATA (sym)->attr.allocatable;
>>              }
>>          }
>> ...
>>         else if (e->symtree)
>>          attr = gfc_variable_attr (e, NULL);
>>
>> So I thought this should already do what you want if you do
>>
>> gfc_is_ptr_fcn (gfc_expr *e)
>> {
>>     return e != NULL && e->expr_type == EXPR_FUNCTION && gfc_expr_attr
>> (e).pointer;
>> }
>>
>> or what am I missing?  The additional checks in gfc_expr_attr are
>> there to avoid ICEs in case CLASS_DATA (sym) has issues, and we all
>> know Gerhard who showed that he is an expert in exploiting this.
>>
>> To sum up, I'd prefer to use the safer form if it works.  If it
>> doesn't, I would expect a latent issue.
>>
>> The rest of the code looked good to me, but I was suspicious about
>> the handling of CHARACTER.
>>
>> Nasty as I am, I modified the testcase to use character(kind=4)
>> instead of kind=1 (see attached).  This either fails here (stop 10),
>> or if I activate the marked line
>>
>> !    cont = tContainer('hello!')       ! ### ICE! ###
>>
>> I get an ICE.
>>
>> Can you have another look?
>>
>> Thanks,
>> Harald
>>
>>>
>>
>>> OK for trunk?
>>>
>>> Paul
>>>
>>> Fortran: Enable class expressions in structure constructors [PR49213]
>>>
>>> 2023-06-24  Paul Thomas  <pault@gcc.gnu.org>
>>>
>>> gcc/fortran
>>> PR fortran/49213
>>> * expr.cc (gfc_is_ptr_fcn): Guard pointer attribute to exclude
>>> class expressions.
>>> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
>>> associate names with pointer function targets to be used in
>>> variable definition context.
>>> * trans-decl.cc (get_symbol_decl): Remove extraneous line.
>>> * trans-expr.cc (alloc_scalar_allocatable_subcomponent): Obtain
>>> size of intrinsic and character expressions.
>>> (gfc_trans_subcomponent_assign): Expand assignment to class
>>> components to include intrinsic and character expressions.
>>>
>>> gcc/testsuite/
>>> PR fortran/49213
>>> * gfortran.dg/pr49213.f90 : New test
> 
> 
> 



  reply	other threads:[~2023-06-27 19:27 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-24 13:18 Paul Richard Thomas
2023-06-24 19:50 ` Harald Anlauf
2023-06-24 19:50   ` Harald Anlauf
2023-06-27 10:30   ` Paul Richard Thomas
2023-06-27 19:27     ` Harald Anlauf [this message]
2023-06-27 19:27       ` Harald Anlauf
2023-06-28  9:47       ` Paul Richard Thomas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=152f9e8b-6998-e505-7875-14dc14268382@gmx.de \
    --to=anlauf@gmx.de \
    --cc=fortran@gcc.gnu.org \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=paul.richard.thomas@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).