Hi Paul, >> The patch actually consists of two parts: >> 1) The resolve.c part prevents the conversion to a PPC call via the >> _vptr (for functions and subroutines). > > This is obviously OK
thanks for the review. >> 2) The class.c parts prevents adding the non-overridable TBP to the vtable. >> >> As noted by Tobias, the second part breaks the ABI, so we might >> consider deferring it until other ABI-breaking features will be >> implemented (cf. http://gcc.gnu.org/wiki/LibgfortranAbiCleanup). On >> the other hand, one could argue that the OOP ABI is still quite young >> and hasn't really stabilized yet (it was broken already from 4.5 to >> 4.6), so we might as well break it again. I know that there are a >> couple of real-world codes out there, which make use of gfortran's OOP >> features already, but I have a hard time estimating how many such >> projects exists, or how problematic an ABI breaking would be for them >> (user input welcome). > > Do we need to exclude it from the _vtable? I have to confess that, > although I tried, I could not think of any reason to exclude it. On > the other hand, I could not see any great harm in retaining a pointer, > albeit a redundant one. Well, no, there is no hard reason why we *need* to exclude it. The point is that, given we call these non-overriable procedure in a "non-vptr" way, there is no need to have them in the vtable either ;) .... except for "historical" / ABI-compatibility reasons. >> So, the question is: Should I commit both parts, or only the resolve.c >> one for now? The patch was regtested on x86_64-unknown-linux-gnu. > > In spite of the above remark, I think that you should commit both > parts. I would in principle also prefer to commit both, but we need to be aware of the risks. In fact I'm not sure whether it is so easy to produce wrong behavior at all, combining code parts that were compiled with different gfortran versions (4.6/4.7). Since the vtabs are supposed to be 'global', usually there should be only one version of them generated in the main program (at least that is how it's supposed to be, but probably that does not work in *every* situation yet). I will try to see if I can find with a failing code example. (The issue is of course that if the vtabs change their contents between 4.6 and 4.7, a type-bound call could end up calling the wrong procedure). Moreover, I have the feeling that NON_OVERRIDABLE is not so widely used "in the wild" (but that is just a feeling, of course). > Perhaps, until just before 4.7 release, a warning should be > triggered that says that pre-existing code containing non-overridable > procedures, should be recompiled before linking? Do you mean the warning should be given on the trunk now, and later taken out of the release? Why not the other way around? > OK for trunk. > > Thanks for the patch Thanks. I will wait a bit before committing to give others a chance to comment. Cheers, Janus
