Community
Participate
Working Groups
Follow-up to bug 247618 - org.eclipse.jdt.core.search.ReferenceMatch says: * This class is not intended to be subclassed by clients. => So shouldn't this class get an @noextend tag and be considered like an internal class now? Which leads us back to field 1 ... with respect to internal superclasses. The dances you introduced with bug 209996 to make setLocalElement(..) inaccessible are striking back again (as I predicted)-: . The protected method ReferenceMatch#localElement() makes little sense (at least after InternalSearchMatch has gone). You should remove localElement() and make getLocalElement() non-final. Or should clients not reimplement localElement()? Then you would have to document this. But if yes, can you then explain how clients of MethodReferenceMatch (which is explicitly intended to be instantiated and subclassed by clients) can create objects of this subclass whose getLocalElement() returns a non-null value? Sorry that I sound a little harsh, but I hate messy APIs, and this one is definitely not yet as clean as it could and should be.
I don't think @noextend means it is internal. API tooling still allows a public class to extend a @noextend class. Darin, am I correct?
@noextend means the class is not intended to be extended outside of the bundle it is defined in. There may be subclasses within the same bundle, and clients may be allowed to instantiate it (unless @noinstantiate), or use instances of it produced by the framework.
The question is whether it's a good idea to have a hierarchy: SearchMatch - intended to be instantiated and subclassed + ReferenceMatch - @noextend + MethodReferenceMatch - intended to be instantiated and subclassed ... and a protected method localElement() in ReferenceMatch. Does that mean that subclasses of MethodReferenceMatch can freely call and override localElement()? If yes, then the method makes little sense. If no, shouldn't API tooling flag such usages of a protected method from an @noextend class, since only subclasses can use that method?
The contract is up to the provider... Whether methods from the superclass can/are intended to be overridden is up to the provider. Methods should be marked as final or @nooverride if they are not intended to be overridden. This is similar to the argument of allowing @noimplement interfaces to be extended and implemented - the contract is up to the provider...
Thanks Darin. Agreed that localElement() should then be marked with @nooverride and @noreference, or we should get rid of it.
(In reply to comment #5) > Agreed that localElement() should then be marked with @nooverride > and @noreference > Released for 3.5M6 in HEAD stream.
Sorry, I have to reopen this. None of the issues of comment 0 have been resolved. > Or should clients not reimplement localElement()? Then you would have to > document this. ReferenceMatch#localElement() is now hidden from clients, but this does not resolve the problems, as I outlined: > But if yes, can you then explain how clients of > MethodReferenceMatch (which is explicitly intended to be instantiated and > subclassed by clients) can create objects of this subclass whose > getLocalElement() returns a non-null value? We either need a setLocalElement(..) method, or all subclasses of ReferenceMatch need a new constructor that also takes a local element. I still vote for the simplest solution (like e.g. for SearchMatch.insideDocComment).
(In reply to comment #7) > > I still vote for the simplest solution (like e.g. for > SearchMatch.insideDocComment). > If I have well understood, your vote would be: 1) make getLocalElement() non-final 2) remove localElement() 3) add setLocalElement(Object) on ReferenceMatch 4) also add a new constructor on ReferenceMatch and it subclasses with an additional argument for the local element (or maybe the setLocalElement(Object) would be enough?)
Created attachment 126015 [details] Proposed patch Does it look better? This patch implements 1-3) but does not add any new constructor as I think there's already enough arguments to existing one...
> Created an attachment (id=126015) [details] > Proposed patch > > Does it look better? Yes, looks great, except for the Javadoc of ReferenceMatch, which still says "[..] not intended to be subclassed [..]", but has no @noextend. I would remove the restriction (otherwise, there would be an "internal" superclass in the hierarchy again). > This patch implements 1-3) but does not add any new constructor as I think > there's already enough arguments to existing one... I fully agree.
Created attachment 126029 [details] New proposed patch Same patch + remove "This class cannot be subclassed by clients..." in ReferenceMatch class javadoc.
Jerome, could let me know if you agree with this proposal? TIA
Comment on attachment 126029 [details] New proposed patch Agreed
(In reply to comment #11) > Created an attachment (id=126029) [details] > New proposed patch > Released for 3.5M6 in HEAD stream.
Verified for 3.5M6
