Hi Andre, >> 1) There are still two TODO markers in the patch. It might be a good >> idea to take care of them before committing the patch. In particular >> for the first one (adding the initializer in gfc_build_class_symbol) >> it would be good to understand where those problems come from. > > I started with the initializer for the _len component and ran into "Pointer > assignment target is neither TARGET nor POINTER at %L" errors (expr.c:3714). I > tracked this back to the constructor resolve of the class type. Resolving the > constructor somehow concludes, that something needs to be done for the > constant > initializer although it is marked artificial. I could not track down the > location that is causing this behavior, or if I need to set a flag in the > class > itself to get through with it. I am hoping, that either some fortran guru says > "You just need to do xyz to get it running." or that we conclude to remove the > TODO and the commented instructions (setting a zero value for _len is done > where > needed (gfc_conv_structure trans-expr.c:6540)).
I can reproduce the "pointer assignment ..." error, but I'm not sure if there is any good way to get rid of it. I'm not even sure if it is a good idea to add an initializer for the _len component at all, since neither _data nor _vptr have one. So, I'm fine with just removing the commented code and the TODO marker, as long as everything works and you make sure the _len component is properly initialized before it is accessed. >> 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? >> 2) You're making a lot of changes to 'trans_associate_var', but I >> don't see any ASSOCIATE statements covered in your test case. Can you >> add more test cases which cover this code? > > Select type (assoc => upoly) uses these where an explicit assoc is supplied. Ah, right. Forgot about that. >> 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. If you want to keep the function, I would either: a) document it more properly, or b) even better: make it more general by calling it like return gfc_get_len_component (e->symtree->n.sym->assoc->target); and inside use gfc_expr *len_comp = gfc_copy_expr (e); Maybe it can be more useful also in other places like this? Cheers, Janus
