New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve Type Resolution #91

Open

ctrimble wants to merge 11 commits into nativelibs4java:master from ctrimble:feature_type-resolution

Contributor

ctrimble commented Jan 18, 2017 •

edited

Loading

NOTE: This PR is not complete, but the test suite is passing. Looking for feedback. Also, it is based on #90, so that should be merged before this PR, or this PR should be rebased.

This PR aims to rework type resolution in BridJ, so that resolution code is off loaded to a third party library and to support more complex generics in classes extending StructObject. Once merged, the following struct will work with BridJ:

  public static abstract class AbstractStructField<S extends StructObject, T extends AbstractStructField<S, T>> extends StructObject {
    @Field(0)
    public S getStruct() {
      return io.getNativeObjectField(this, 0);
    }
    
    public T setStruct( S struct ) {
      io.setNativeObjectField(this, 0, struct);
      return (T)this;
    }
  }
  
  public static class StructOfInteger extends StructObject {
    @Field(0)
    public int getField() {
      return io.getIntField(this, 0);
    }
    
    public StructOfInteger setField( int field ) {
      io.setIntField(this, 0, field);
      return this;
    }
  }
  
  // this is the struct that will now work as expected.
  public static class NestedStructOfInteger
    extends AbstractStructField<StructOfInteger, NestedStructOfInteger> {}

Status: Code to support generic resolution has been introduced and a test called StructGenericsTest has been added to verify that structures of this type now work.

TODO: Remove dead code dealing directly with Java's type classes.

Build Changes:

The library supporting type resolution, ClassMate, is compiled for Java 1.6, so Java 1.5 compatibility has been abandoned.
The source version has been moved to 1.8 and Retrolambda has been introduced, to produce classes compatible with Java 6.
The version has been bumped to 0.8.0-SNAPSHOT, to reflect the changes in the resulting artifacts.

Fixes #59

ctrimble added 2 commits

January 14, 2017 22:58


          turn off doclint in the java 8 compiler

5887a80


          ignore .DS_Store files created by OSX

bc17c24

ctrimble commented

View reviewed changes

pom.xml

+                                                <target>${maven.compiler.target}</target>
+                                                <testSource>${maven.compiler.testSource}</testSource>
+                                                <testTarget>${maven.compiler.testTarget}</testTarget>
+              				</configuration>

Contributor Author

ctrimble Jan 18, 2017

The parent pom should not be setting these to 1.5. Instead, it should define these properties to 1.5, allowing child poms to override them, without having to redefine the plugin.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

               class StructFieldDeclaration {
                   final StructFieldDescription desc = new StructFieldDescription();
                   Method setter;
+                  ResolvedMember resolvedSetter;

Contributor Author

ctrimble Jan 18, 2017

This type should be ResolvedMethod. Also, it would greatly simplify the code base if this was a final field, and it was some type of Consumer<?>. Not sure about performance impact with that.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

@@ @@ -58,6 +76,7 @@ public String toString() { @@
                       return desc.name + " (index = " + index + (unionWith < 0 ? "" : ", unionWith = " + unionWith) + ", desc = " + desc + ")";
                   }
+                  @Deprecated
                   protected static boolean acceptFieldGetter(Member member, boolean getter) {

Contributor Author

ctrimble Jan 18, 2017

Remove this, dead code.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

                   protected static List<StructFieldDeclaration> listFields(Class<?> structClass) {
                       List<StructFieldDeclaration> list = new ArrayList<StructFieldDeclaration>();
                       for (Method method : structClass.getMethods()) {
                           if (acceptFieldGetter(method, true)) {
                               StructFieldDeclaration io = fromGetter(method);
                               try {
+                                  // this only works when the names are equal, does not support setXXX methods.

Contributor Author

ctrimble Jan 18, 2017

Add a test to show that this is broken and fix it.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

                       return list;
                   }
+                  protected static List<StructFieldDeclaration> listFields2(Class<?> structClass) {

Contributor Author

ctrimble Jan 18, 2017

get this into a separate class, it does not belong here.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                  }
+                  protected static ResolvedTypeWithMembers resolveType( Class<?> structClass ) {
+                    TypeResolver resolver = new TypeResolver();

Contributor Author

ctrimble Jan 18, 2017

There should just be one of these.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                    TypeResolver resolver = new TypeResolver();
+                    ResolvedType classType = resolver.resolve(structClass);
+                    MemberResolver mr = new MemberResolver(resolver);
+                    mr.setMethodFilter(new Filter<RawMethod>() {

Contributor Author

ctrimble Jan 18, 2017

make lambda

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                          !method.isStatic();
+                      }
+                    });
+                    mr.setFieldFilter(new Filter<RawField>() {

Contributor Author

ctrimble Jan 18, 2017

make lambda

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                    decl.desc.isSizeT = member.get(org.bridj.ann.Ptr.class) != null;
+                  }
+                  @Deprecated
                   protected static StructFieldDeclaration fromField(java.lang.reflect.Field getter) {

Contributor Author

ctrimble Jan 18, 2017

dead code, remove

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                      return field;
+                  }
+                  @Deprecated
                   protected static StructFieldDeclaration fromGetter(Method getter) {

Contributor Author

ctrimble Jan 18, 2017

Dead code, remove.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                    return field;
+                }
+                  @Deprecated
                   private static StructFieldDeclaration fromMember(Member member) {

Contributor Author

ctrimble Jan 18, 2017

Dead code, remove.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDeclaration.java Outdated

+                    return mr.resolve(classType, annConfig, null);
+                  }
+                  protected static <T extends Member> void updateDecl(StructFieldDeclaration decl, ResolvedMember<T> member ) {

Contributor Author

ctrimble Jan 18, 2017

Is this dead code? There is another version below.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDescription.java

+                        ResolvedType rt = (ResolvedType)tpe;
+                        // TODO: what do we do here?
+                        ret = tpe;
+                      }

Contributor Author

ctrimble Jan 18, 2017

Can this actually be anything other than ResolvedType now?

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructFieldDescription.java

+                                  field.desc.nativeTypeOrPointerTargetType = tpe;
+                                }
+                              }
+                              else if(field.desc.valueType instanceof ParameterizedType) {

Contributor Author

ctrimble Jan 18, 2017

This should be dead code.

ctrimble commented

View reviewed changes

src/main/java/org/bridj/StructUtils.java Outdated

@@ @@ -227,7 +227,7 @@ static String describe(Type t) { @@
                   @SuppressWarnings("deprecation")
                   protected static void computeStructLayout(StructDescription desc, StructCustomizer customizer) {
-                      List<StructFieldDeclaration> fieldDecls = StructFieldDeclaration.listFields(desc.structClass);
+                      List<StructFieldDeclaration> fieldDecls = StructFieldDeclaration.listFields2(desc.structClass);

Contributor Author

ctrimble Jan 18, 2017

Fix this name after removing original implementation.

ctrimble commented

View reviewed changes

src/test/velocity/org/bridj/PointerTest.java

               	@Test
                   public void testPointerTo_${prim.Name}_Values() {
               		// Test pointerToInts(int...)
-              		Pointer<${prim.rawTypeRef}> p = Pointer.pointerTo${prim.CapName}s(${prim.value($v1)}, ${prim.value($v2)}, ${prim.value($v3)});
+              		Pointer<${rawTypeRef}> p = Pointer.pointerTo${prim.CapName}s(${prim.value($v1)}, ${prim.value($v2)}, ${prim.value($v3)});

Contributor Author

ctrimble Jan 18, 2017

The changes here fix generic usage and are not changes to functionality.

ctrimble commented

View reviewed changes

src/test/velocity/org/bridj/PointerTest.java

               	}
               	@Test
                   public void testPointerTo_${prim.Name}_Value() {
-              		Pointer<${prim.rawTypeRef}> p = Pointer.pointerTo${prim.CapName}(${prim.value($v1)});
+              		Pointer<${rawTypeRef}> p = Pointer.pointerTo${prim.CapName}(${prim.value($v1)});

Contributor Author

ctrimble Jan 18, 2017

The changes here fix generic usage and are not changes to functionality.

ctrimble force-pushed the feature_type-resolution branch from f713cba to 2736544 Compare

January 20, 2017 03:16

ctrimble added 3 commits

January 19, 2017 20:18


          add classmate and retrolambda to the pom.

cf0c697


          corrected issues with Pointer in generated PointerTest

7662f56


          first pass at adding classmate based resolution. Test suite passing.

07a9235

Old code is still present.

ctrimble force-pushed the feature_type-resolution branch from 2736544 to 07a9235 Compare

January 20, 2017 03:19

ctrimble added 6 commits

January 19, 2017 20:29


          removed original resolution code

88867c1


          fixed warnings

1994d5f

renamed resolution method to origin name


          removed tests comparing origin and new resolution code.

c7ae5f6


          moved type resolution code out of struct delcaration class.

d494ad9


          added source declaration to the javadoc plugin

2cec0c1


          SQUASH: added missing StructIO changes to resolver refactoring

cbe4a42

Contributor Author

ctrimble commented Jan 20, 2017

I scanned the code base for ParameterizedType and attempted to remove it. There is still a lot of resolution code happening in CRuntime and its subclasses. I would like to remove ParameterizedType, TypeVariable, etc from the codebase, to make sure resolution is consistent, but it may need to happen over several PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet