Community
Participate
Working Groups
Build Identifier: CVS Head of 2010-11-10 The getDelegateDomain operation doesn't use the public getURI() method which subclasses may redefine. Therefore, redefinitions would have no effect on getDelegateDomain(). The patch attached fixes this. Reproducible: Always
Created attachment 182889 [details] Fix using getURI() in getDelegateDomain(...)
Created attachment 183519 [details] Add final Mmm. Not so sure. A getXXX() method may have two purposes a) make XXX accessible b) make getXXX overrideable if a) it's WONTFIX if b) it's a bug How do we know what the API intent is? By the presence/absence of a final on getURI(). Bug 330734 raised to diagnose this problem. Here the access is to a protected final field so the intention was clearly to just provide read access. Replacement patch just adds final.
Axel, It could be interesting to me knowing if there were a use case in which overriding getURI is useful since it's the delegateDomain that should provide said URI to the factory. Axel and Ed comments make some sense, however, we can't simply make the method final since we were breaking clients who were already overriding the method. +1 to Axel's patch, maybe including the "@nooverride This method is not intended to be re-implemented or extended by clients." in the getURI() method javadoc to warn users that this method should not be overriden. Regards, Adolfo.
(In reply to comment #3) > Axel and Ed comments make some sense, however, we can't simply make the method > final since we were breaking clients who were already overriding the method. I was halfway through -1'ing my patch for exactly this API compatibility, when I checked the API tooling and this is seemingly not an API issue. The problem is probably only at source level. Rebuilding an override of the final will fail. I suppose that executing the override of the final may be ignored, but there is a risk that class loading may fail. Just adding @nooverride documents the intended design without breaking overriding clients and without changing the design, which a similarly tiny number of clients may depend on. Axel: is there a use case for overriding getURI()?
> Axel: is there a use case for overriding getURI()? No, this was just an observed potential inconsistency in case someone chose to override getURI() in a subclass, possibly expecting it to have an impact. Disallowing a redefinition of getURI() by making it final to me seems like an acceptable solution and is in line with the "final-ness" of the delegateURI field.
We do not have agreement here. Adding "final" requires an API filter, suggesting a remote possibility that someone might get broken. Adding "@nooverride" has no such problem. My first preference is to add @nooverride. My second preference is to do nothing.
I have the same feelings and the same preferences. Providing that your first preference have also two alternatives: 1.a including Axel's change. 1.b no including axel's change. My preferences are the following: firstly 1.a, then 1.b, finally your second preference. Regards, Adolfo.
Adolfo: Your comments confuse me again. One the one hand you agree with my preferences but then you say to include Axel's change, which is ambiguous and contradictory. Axel's first suggestion was to eliminate the bypass of getURI(). This is neither of my preferences. Axel's later suggestion was to add final which has an API risk. This again is neither of my preferences. Which of Axel's changes do you want and do you understand the problems?
(In reply to comment #8) > Adolfo: Your comments confuse me again. > > One the one hand you agree with my preferences but then you say to include > Axel's change, which is ambiguous and contradictory. > > Axel's first suggestion was to eliminate the bypass of getURI(). This is > neither of my preferences. > > Axel's later suggestion was to add final which has an API risk. This again is > neither of my preferences. > > Which of Axel's changes do you want and do you understand the problems? Ed, Your first preference implicitly discards "Axel's later suggestion", so my 1.a preference can only refers to "Axel's first suggestion". Actually, I originally recalled (comment 3) for said 1.a preference: - Including the annotation to warn that the method is not intended to be overriden. - If somebody don't want to follow the suggestion, using a derived OCL DelegateFactory which overrides the getURI method with its own URI, which hypothetically should correspond to its own delegate domain. Would the client want to have the default OCL delegate domain for its own derived delegate factory ?. Giving the fact that we must adjust to Eclipse API related policy, which discards the best solution (the final modifier on the method), for consistency I prefer 1.a rather than 1.b. Regards, Adolfo.
I'm fine with @nooverride.
I hate this bug. It is totally trivial and irrelevant and wasting a lot of time. Adolfo: I do not understand what you want. Please submit a patch.
Ed, Just commit the @noverride. Regards, Adolfo.
@nooverride copmmitted to head for 3.1.0M5
Also committed to R3_0_maintenance branch (I'll also sieze the opportunity to check the maintenance branch build)
-1 Please revert. This is not a maintenance patch. It fixes no bugs for 3.0 users. It has a minor source code incompatibility, which should not be imposed on old users. [You may recall that for 1.3.0SR1 we did not commit the patch that fixed a few hundred warnings since it too had no maintenance benefit.]
To clarify the reversion. This patch should be present in 3.1.0 but not in 3.0.2.
Ed, I believe that we are not imposing anything to users. Just warning that the said method should not be overriden. It simply increases the documentation of the method which I think it's useful for clients... Why do we want to wait until Indigo to warn our clients ?. Furthermore, if they override in Helios they could experiment unexpected behaviour...so we are in somehow fixing a bug, in this case changing the javadoc. Anyway, if my reasoing doesn't convince you, please have a look the following link: http://wiki.eclipse.org/Version_Numbering#When_to_change_the_service_segment Let me know if you still want to revert to maintenance branch change. Best Regards, Adolfo.
I give up. I cannot find any Wiki guidance on Service Release contents. This change is very unlikely to help anybody. This change is very unlikely to harm anybody. So I guess it's not worth arguing any longer about whether this is in 3.0.2 or not.
Closing all bugs resolved in Indigo.
