Re: [PATCH, fortran] PR fortran/60255 Deferred character length

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Dear Andre,

I am open to either - what do the others think?

The reason why I am for differentiating between unlimited_polymorphic
objects and the deferred length character arrays is because of the
difference in the way in which arrays are accessed. The former uses
pointer arithmetic and the latter array references. I was trying to
avoid divisions by KIND within scalarization loops. Also, I found that
in developing your patch, that allocating with unlimited polymorphic
sources looks neatest when the _len contains the memory size of the
elements of any dynamic type, since a priori it is not known at
compile time whether it is a character or not. Of course, one could
interrogate the _hash field of the vtable, at the expense of more
runtime code.

Cheers

Paul

PS I have your patches for PR60357 and 61275 regtesting right now.
Both look OK to me. At the risk of making potential regressions more
complicated to unravel, to save my time I intend to commit both at
once, unless anybody objects.



On 17 January 2015 at 13:10, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Paul,
>
> I am open on what to call the new component.
>
> Have you thought about my findings, that for deferred length char arrays the
> length is stored in characters and not in bytes, I.e., for a
> character(kind=4, Len=:) the length is stored in number of characters and
> not in bytes needed, which would be Len*4. IMHO both concepts should be
> changed, or none. I favor to keep storing the string length of both concepts
> (deferred char arrays and chararrays in unlimited polymorphic entities)
> interchangeable w/o computation.
>
> What's your opinion?
>
> Regards, Andre
>
> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
> Tel. +49 241.9291018 * vehre@gmx.de
>
>
> Paul Richard Thomas <paul.richard.thomas@gmail.com> schrieb:
>
>
> Dear Andre,
>
> Perhaps, rather than calling the new component _len, we should call it
> _mem_size or some such?
>
> Cheers
>
> Paul
>
> On 9 January 2015 at 11:52, Andre Vehreschild <vehre@gmx.de> wrote:
>> Hi all, hi Paul,
>>
>> I started to implement the changes requested below, but I stumbled over an
>> oddity:
>>
>> For a deferred length kind4 char array, the length of the string is stored
>> without multiplication by 4 in the length variable attached. So when we
>> now
>> decide to store the length of the string in an unlimited polymorphic
>> entity in
>> bytes in the component formerly called _len and the size of each character
>> in
>> _vtype->_size then we have an inconsistency with the style deferred char
>> lengths are stored. IMHO we should store this consistently, i.e., both
>> 'length'-variables store either the length of the string ('length' =
>> array_len)
>> or the size of the memory needed ('length' = array_len * char_size). What
>> do
>> you think?
>>
>> Furthermore, think about debugging: When looking at an unlimited
>> polymorphic
>> entity storing a kind-4-char-array of length 7, then having a 'length'
>> component
>> set to 28 will lead to confusion. I humbly predict, that this will produce
>> many
>> entries in the bugtracker, because people don't understand that 'length'
>> stores
>> the product of elem_size times string_len, because all they see is an
>> assignment of a length-7 char array.
>>
>> What do we do about it?
>>
>> Regards,
>>         Andre
>>
>> On Thu, 8 Jan 2015 20:56:43 +0100
>> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>>
>>> Dear Andre,
>>>
>>> Thanks for the patch. As I have said to you, off list, I think that
>>> the _size field in the vtable should contain the kind information and
>>> that the _len field should carry the length of the string in bytes. I
>>> think that it is better to optimise array access this way than to
>>> avoid the division in evaluating LEN (). I am happy to accept contrary
>>> opinions from the others.
>>>
>>> I do not believe that the bind_c issue is an issue. Your patch
>>> correctly deals with it IMHO.
>>>
>>> Subject to the above change in the value of _len, I think that your
>>> patch is OK for trunk.
>>>
>>> With best regards
>>>
>>> Paul
>>>
>>> On 4 January 2015 at 13:40, Andre Vehreschild <vehre@gmx.de> wrote:
>>> > Hi Janus, hi Paul, hi Tobias,
>>> >
>>> > Janus: During code review, I found that I had the code in
>>> > gfc_get_len_component() duplicated. So I now reintroduced and
>>> > documented the
>>> > routine making is more commonly usable and added more documentation.
>>> > The
>>> > call sites are now simplify.c (gfc_simplify_len) and trans-expr.c
>>> > (gfc_trans_pointer_assignment). Attached is the reworked version of the
>>> > patch.
>>> >
>>> > Paul, Tobias: Can one of you have a look at line 253 of the patch? I
>>> > need
>>> > some expertise on the bind_c behavior. My patch needs the check for
>>> > is_bind_c added in trans_expr.c (gfc_conv_expr) to prevent mistyping an
>>> > associated variable in a select type() during the conv. Background:
>>> > This
>>> > code fragment taken from the testcase in the patch:
>>> >
>>> > MODULE m
>>> > contains
>>> >   subroutine bar (arg, res)
>>> >     class(*) :: arg
>>> >     character(100) :: res
>>> >     select type (w => arg)
>>> >       type is (character(*))
>>> >         write (res, '(I2)') len(w)
>>> >     end select
>>> >   end subroutine
>>> > END MODULE
>>> >
>>> > has the conditions required for line trans-expr.c:6630 of gfc_conv_expr
>>> > when
>>> > the associate variable w is converted. This transforms the type of the
>>> > associate variable to something unexpected in the further processing
>>> > leading to some issues during fortraning. Janus told me, that the
>>> > f90_type
>>> > has been abused for some other things (unlimited polymorphic
>>> > treatment).
>>> > Although I believe that reading the comments above the if in question,
>>> > the
>>> > check I had to enhance is treating bind_c stuff (see the threads
>>> > content
>>> > for more). I would feel safer when one of you gfortran gurus can have a
>>> > look and given an opinion, whether the change is problematic. I
>>> > couldn't
>>> > figure why w is resolved to meet the criteria (any ideas). Btw, all
>>> > regtest
>>> > are ok reporting no issues at all.
>>> >
>>> > Bootstraps and regtests ok on x86_64-linux-gnu
>>> >
>>> > Regards,
>>> >         Andre
>>> >
>>> >
>>> > On Sat, 3 Jan 2015 16:45:07 +0100
>>> > Janus Weil <janus@gcc.gnu.org> wrote:
>>> >
>>> >> Hi Andre,
>>> >>
>>> >> >> >> For the
>>> >> >> >> second one (in gfc_conv_expr), I don't directly see how it's
>>> >> >> >> related
>>> >> >> >> to deferred char-len. Why is this change needed?
>>> >> >> >
>>> >> >> > That change is needed, because in some rare case where an
>>> >> >> > associated
>>> >> >> > variable in a "select type ()" is used, then the type and
>>> >> >> > f90_type
>>> >> >> > match the condition while them not really being in a bind_c
>>> >> >> > context.
>>> >> >> > Therefore I have added the check for bind_c. Btw, I now have
>>> >> >> > removed
>>> >> >> > the TODO, because that case is covered by the regression tests.
>>> >> >>
>>> >> >> I don't understand how f90_type can be BT_VOID without being in a
>>> >> >> BIND_C context, but I'm not really a ISO_C_BINDING expert. Which
>>> >> >> test
>>> >> >> case is the one that triggered this?
>>> >> >
>>> >> > This case is triggered by the test-case in the patch, where in the
>>> >> > select
>>> >> > type (w => arg) in module m routine bar the w meets the criteria to
>>> >> > make
>>> >> > the condition become true. The type of w is then "fixed" and
>>> >> > gfortran
>>> >> > would terminate, because the type of w would be set be and
>>> >> > BT_INTEGER. I
>>> >> > tried to backtrace where this is coming from, but to no success. In
>>> >> > the
>>> >> > resolve () of the select type it looks all quite ok, but in the
>>> >> > trans
>>> >> > stage the criteria are met. Most intriguing to me is, that in the
>>> >> > condition we are talking about the type of w and f90_type of the
>>> >> > derived
>>> >> > class' ts (expr->ts.u.derived->ts.f90_type) of w is examined. But
>>> >> > expr->ts.u.derived->ts does not describe the type of w, but of the
>>> >> > class
>>> >> > w is associate with __STAR...
>>> >> >
>>> >> > So I am not quite sure how to fix this, if this really needs fixing.
>>> >> > When I understand you right, then f90_type should only be set in a
>>> >> > bind_c context, so adding that check wouldn't hurt, right?
>>> >>
>>> >> Yes, in principle adding the check for attr.bind_c looks ok to me
>>> >> (alternatively one could also check for attr.unlimited_polymorphic). I
>>> >> think originally BT_VOID was indeed only used in a bind_c context, but
>>> >> recently it has also been 'hijacked' for unlimited polymorphism, e.g.
>>> >> for the STAR symbol and some of the components of the intrinsic vtabs.
>>> >>
>>> >> What I don't really understand is why these problems are triggered by
>>> >> your patch now and have not crept up earlier in other use-cases of
>>> >> CLASS(*).
>>> >>
>>> >>
>>> >> >> >> 3) The function 'gfc_get_len_component' that you're introducing
>>> >> >> >> is
>>> >> >> >> only called in a single place. Do you expect this to be useful
>>> >> >> >> in
>>> >> >> >> other places in the future, or could one remove the function and
>>> >> >> >> insert the code inline?
>>> >> >> >
>>> >> >> > In one of the first versions it was uses from two locations. But
>>> >> >> > I
>>> >> >> > had to remove one call site again. I am currently not sure, if I
>>> >> >> > will
>>> >> >> > be using it in the patch for allocatable components when deferred
>>> >> >> > char arrays are handled. So what I do I do now? Inline it and
>>> >> >> > when
>>> >> >> > needed make it explicit again in a future patch?
>>> >> >>
>>> >> >> I leave that up to you. In principle I'm fine with keeping it as it
>>> >> >> is. The only problem I see is that the function name sounds rather
>>> >> >> general, but it apparently expects the expression to be an
>>> >> >> ASSOCIATE
>>> >> >> symbol.
>>> >> >
>>> >> > I am nearly finished with the patch on allocatable scalar components
>>> >> > and
>>> >> > I don't need the code there. Therefore I have inlined the routine.
>>> >>
>>> >> Ok, good. Could you please post an updated patch?
>>> >>
>>> >>
>>> >> > So, what do we do about the bind_c issue above? Is some bind_c guru
>>> >> > available to have a look at this? It would be very much appreciated.
>>> >>
>>> >> From my non-guru POV, it can stay as is.
>>> >>
>>> >> It would be helpful if someone like Paul or Tobias could have a look
>>> >> at the patch before it goes to trunk. I think it's pretty close to
>>> >> being ready for prime-time. Thanks for your work!
>>> >>
>>> >> Cheers,
>>> >> Janus
>>> >
>>> >
>>> > --
>>> > Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
>>> > Tel.: +49 241 9291018 * Email: vehre@gmx.de
>>>
>>>
>>>
>>
>>
>> --
>> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
>> Tel.: +49 241 9291018 * Email: vehre@gmx.de
>
>
>
> --
> Outside of a dog, a book is a man's best friend. Inside of a dog it's
> too dark to read.
>
> Groucho Marx



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx