Community
Participate
Working Groups
20030227 javadoc for IMethod.isSimilar does not check if isConstructor() is the same for both methods i think it would make sense
This is a breaking API change. Not sure we want to do it that late in the 2.1 release cycle. Having isContructor() should be enough. isSimilar() is only checking parameter's simple names anyway, so two methods can be considered as similar without being the same.
post 2.1 is ok for me
Defer for post 2.1.
reopen - unclear we want to break API (unless implementation is clearly wrong). May want to introduce a new API for this.
Jim - any thoughts on this change?
Given the way all of IMethod is spec'd, isConstructor is significant and the implementation should be taking it into account. That is, this is just a bug. Fixing it does not require changing the API.
Change the javadoc to: /** * Returns whether this method is similar to the given method. * Two methods are similar if: * <ul> * <li>their element names are equal</li> * <li>their isConstructor() values are equal</li> * <li>they have the same number of parameters</li> * <li>the simple names of their parameter types are equal</li> * </ul> * This is a handle-only method. * * @param method the given method * @return true if this method is similar to the given method. * @see Signature#getSimpleName * @since 2.0 */ boolean isSimilar(IMethod method); Change the current implementation for: public boolean isSimilar(IMethod method) { try { if (this.isConstructor() != method.isConstructor()) { return false; } } catch(JavaModelException e) { } return this.areSimilarMethods( this.getElementName(), this.getParameterTypes(), method.getElementName(), method.getParameterTypes(), null); } Does it look ok for you?
Created attachment 4468 [details] Patch using the changes described above
code looks good (i think it's too risky to put for 2.1.1, however) thanks
One spec suggestion: Rather than mention method by name: * <li>their isConstructor() values are equal</li> the same thing can be said less formally using words like: * <li>they are both methods or both constructors</li>
Thanks Jim. I will change it. Adam - Why this is too risky for 2.1.1?
it might not be trivial to asses how and where we rely on the fact that it ignores the isConstructor i can have a look if you really want to put it in for 2.1.1, which i think it's unnecessary - not a severe bug, is it?
Ok, fixed and released only in 2.2 stream.
Reopen. In fact we cannot fix it because isConstructor populates the java model.
Close as WONTFIX. This method cannot populate the java model. So we cannot call isConstructor() within this method.
Right. I didn't notice that isConstructor() is a handle-only method and that constructor-ness is not stored in the handle. Note that the chances of a constructor handle clashing with a method handle are probably rather low in practice given standard naming conventions.
ok, thanks anyway i don't use that method now but i will put a workaround if i need to (got used by now :-))
