Bug 331558

Comment 1 Leslie Davis 2010-12-21 14:35:43 EST

Created attachment 185665 [details]
proposed bug fix patch

Attached a proposed bug fix patch.

Comment 2 Neil Hauge 2010-12-23 12:53:54 EST

Here is some feedback on the patch, some of which we have discussed.  I haven't actually tested the patch yet, so these are just code observations.


JDTTools.
typeImplementsInterface(IJavaProject javaProject, IType type, IType interfase)
 - the second condition tested (typeName.equals(interfaceName)) should be a separate error case, not an answer of true to this method.

resolveSuperInterfaceNames(...)
 - returned Iterable will only contain the first match for any set of resolved types.  All matches should be added to cover the case where the type short name is ambiguous.

- regarding the use of type.getSuperInterfaceNames().  After considering the trade-offs between using this method versus an ITypeHierarchy, I think using the method from the patch is reasonable given its performance benefits.  The creation of a TypeHierarchy object can be long running, and as a result should only be done if necessary.  This approach may have to be reconsidered in the future if gaps are found in the current approach.

o.e.j.eclipselink.core.internal.context.orm
- with these changes, it appears to be time to create a new class in the hierarchy to represent converters where a converter class is specified.  This patch introduces a decent amount of duplicate code (in the validation logic for OrmEclipseLinkCustomConverter and OrmEclipseLinkStructConverter) and there are other parts of these two classes that are already the same.  I suggest creating an abstract OrmEclipseLinkConverterClassConverter class to hold the common code between the 2 classes mentioned above.  This should remove a good deal of duplicate code between the two classes.

o.e.j.eclipselink.core.internal.resource.java.*
o.e.j.eclipselink.core.resource.java
- The changes contained in these packages appear to be from an earlier attempt at a solution.

Comment 3 Leslie Davis 2011-01-06 14:12:31 EST

Created attachment 186213 [details]
patch with additional recommended changes

Comment 4 Neil Hauge 2011-01-10 17:34:19 EST

(In reply to comment #3)
> Created attachment 186213 [details]
> patch with additional recommended changes

There appears to be a problem getting a text range for the StructConverter case for Java.  I am getting the right problem in the Problems view, but not in the editor, and the problem in the Problems view does not indicate a line location.

Comment 5 Leslie Davis 2011-01-12 18:02:03 EST

Created attachment 186690 [details]
proposed bug fix patch

fixed issue with text range.

Comment 6 Neil Hauge 2011-01-18 10:23:16 EST

As we discussed, there is some duplicate code inside ASTTools after applying the patch.  Please refactor ASTTools so that this duplicate code is eliminated.

Comment 7 Leslie Davis 2011-01-20 14:19:08 EST

Created attachment 187228 [details]
proposed bug fix patch

Implemented suggested re-factoring

Comment 8 Neil Hauge 2011-01-20 18:12:48 EST

Patch has been committed to head.