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 -- Outside of a dog, a book is a man's best friend. Inside of a dog it's too dark to read. Groucho Marx
