Dear Andre,
Don't worry about the lack of a laptop. I am on to it :-)
Paul
On 17 January 2015 at 16:14, Andre Vehreschild <ve...@gmx.de> wrote:
> Dear Paul,
>
> Setting _Len to one by default should be quite simple. When I remember
> correctly, then there is only one place where the current code sets it to
> zero. Could be in gfc_conv_structure but that is only a guess.
> Unfortunately am I on travel 'till Sunday and don't have my laptop with me.
> Sorry for that.
>
> Regards,
> Andre
>
> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
> Tel. +49 241.9291018 * ve...@gmx.de
>
>
> Paul Richard Thomas <paul.richard.tho...@gmail.com> schrieb:
>
> Cancel that - It should be multiplies by kind, shouldn't it ? :-) OK,
> string length it is. We will probably have to set _len = 1 for other
> dynamic types, though, so that the pointer arising from an array
> reference is base_address + _len*vptr->size*index
>
> Cheers
>
> Paul
>
> On 17 January 2015 at 13:44, Paul Richard Thomas
> <paul.richard.tho...@gmail.com> wrote:
>> 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 <ve...@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 * ve...@gmx.de
>>>
>>>
>>> Paul Richard Thomas <paul.richard.tho...@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 <ve...@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.tho...@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 <ve...@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 <ja...@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: ve...@gmx.de
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
>>>> Tel.: +49 241 9291018 * Email: ve...@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
>
>
>
> --
> 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
