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