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
