Skip to content
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

Feature/autodowncast4 #554

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature/autodowncast4 #554

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8d4e55
Fix index out of bounds error
Feb 9, 2022
5134257
First attempt at generated auto downcast based on c++ type_index
Feb 9, 2022
a570b77
Improved Java 1.7 compatibility by not using 'var'
Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/main/java/org/bytedeco/javacpp/annotation/Downcast.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.bytedeco.javacpp.annotation;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD, ElementType.PARAMETER })
public @interface Downcast {
String base() default "";

boolean classcache() default false;
}
144 changes: 141 additions & 3 deletions src/main/java/org/bytedeco/javacpp/tools/Generator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@
import org.bytedeco.javacpp.annotation.ValueSetter;
import org.bytedeco.javacpp.annotation.Virtual;
import sun.misc.Unsafe;
import java.lang.invoke.MethodHandleInfo;
import org.bytedeco.javacpp.annotation.Downcast;

/**
* The Generator is where all the C++ source code that we need gets generated.
Expand Down Expand Up @@ -181,6 +183,124 @@ static enum LongEnum { LONG; long value; }
Map<Method,MethodInformation> annotationCache;
boolean mayThrowExceptions, usesAdapters, passesStrings, accessesEnums;

private void outputDowncastFunDeclaration() {
out.println(""); // a forward declaration is necessary because generated JNI methods will need to call the downcast function
out.println("extern \"C++\" template<class BASE> jobject JavaCPP_downcast(JNIEnv* env, BASE* obj);");
out.println("");
}

private void outputDowncastFunDefinition() {
out.println("");
out.println("#define JAVACPP_DEBUG_DCAST false");
out.println("");
out.println("#include <typeindex>");
out.println("#include <typeinfo>");
out.println("#include <ostream>");
out.println("#include <iostream>");
out.println("#include <unordered_map>");
out.println("#include <utility>");
out.println("");
out.println("typedef struct constructor {");
out.println(" jclass clazz;");
out.println(" jmethodID ctor_mid;");
out.println("} constructor;");
out.println("");
out.println("typedef std::unordered_map<std::type_index, constructor> JavaCPP_JavaCPP_downcast_cache_t;");
out.println("");
out.println("JavaCPP_JavaCPP_downcast_cache_t JavaCPP_downcast_cache;");
out.println("");
out.println("extern \"C++\" template<class BASE> jobject JavaCPP_downcast(JNIEnv* env, BASE* obj) {");
out.println(" std::type_index id = typeid(*obj);");
out.println(" JavaCPP_JavaCPP_downcast_cache_t::iterator ctor = JavaCPP_downcast_cache.find(id);");
out.println(" if (ctor != JavaCPP_downcast_cache.end()) {");
out.println(" jobject ret = (jobject)env->NewObject(ctor->second.clazz, ctor->second.ctor_mid, NULL);");
out.println(" if (ret == NULL || env->ExceptionCheck()) {");
out.println(" std::cerr << \"Error calling Pointer constructor \" << std::endl;");
out.println(" return NULL;");
out.println(" }");
out.println(" if (JAVACPP_DEBUG_DCAST) std::cerr << \"Success! \" << std::endl;");
out.println(" return ret;");
out.println(" }");
out.println(" else");
out.println(" return NULL;");
out.println("}");
out.println("");
out.println("void JavaCPP_dcast_preload(JNIEnv* env, std::string class_name, std::type_index type_id) {");
out.println(" if (JAVACPP_DEBUG_DCAST) std::cerr << \"Try to find class \" << class_name << std::endl;");
out.println("");
out.println(" jclass lclazz = env->FindClass(class_name.c_str());");
out.println(" if (lclazz == NULL || env->ExceptionCheck()) {");
out.println(" std::cerr << \"Error loading class \" << class_name.c_str() << std::endl;");
out.println(" return;");
out.println(" }");
out.println("");
out.println(" jclass gclazz = (jclass)env->NewGlobalRef(lclazz);");
out.println(" if (gclazz == NULL || env->ExceptionCheck()) {");
out.println(" std::cerr << \"Error creating class \" << class_name << std::endl;");
out.println(" return;");
out.println(" }");
out.println(" ");
out.println(" if (JAVACPP_DEBUG_DCAST) std::cerr << \"Try to call constructor for class \" << class_name << std::endl;");
out.println(" jmethodID ctor_mid = env->GetMethodID(gclazz, \"<init>\", \"(Lorg/bytedeco/javacpp/Pointer;)V\");");
out.println(" if (ctor_mid == NULL || env->ExceptionCheck()) {");
out.println(" std::cerr << \"Error getting Pointer constructor \" << class_name << std::endl;");
out.println(" return;");
out.println(" }");
out.println(" JavaCPP_downcast_cache[type_id] = constructor{gclazz,ctor_mid};");
out.println(" }");
}

void outputDowncastCache(LinkedHashSet<Class> allClasses) {
var baseClasses = new LinkedHashSet<String>();
saudet marked this conversation as resolved.
Show resolved Hide resolved
// Scan to find out Downcast base classes
for (Class<?> cls : allClasses) {
// if super class is InfoMapper then this is likely a target class else skip this
if (!InfoMapper.class.isAssignableFrom(cls.getSuperclass()))
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also make this work for classes written manually that are not meant to parse header files, that is to say, they do not implement InfoMapper.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, how do we go about that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose if we put annotations only on the base classes we're not going to need this logic, am I right?

// check all methods to see if they declare a Downcast baseclass via annotation
for (Method m : cls.getMethods()) {
Downcast d = m.getAnnotation(Downcast.class);
if (d != null && d.base().length() > 0)
baseClasses.add(d.base());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why the annotation is on the methods. Why can't it be on the base class itself, and that's it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it should be on the base class. I'm all for it. But I could not figure out how to do that, everything I tried caused some or other problem in parsing or the generated Java code.

So I could do with some help here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Opaque is an annotation used only the class. Look at where that and others like that get used. Let me know if you encounter any specific problem though, and I'll look into it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find any examples (my bad I'm sure) how to apply @Opaque to a class and looking at source code where this is emitted did not make the penny drop either, so to experiment with this I tried:

class Foo  {
        public:
                virtual void bar();
};

and added

		infoMap.put(new Info("Foo").annotations("@Opaque"));

but this results in error:

Info: Parsing FakeOccDemoApi.hxx
Info: javac -cp /Users/nyholku/javacpp/target/classes:/Users/nyholku/javacpp/jars/junit-4.13.2.jar:/Users/nyholku/javacpp/jars/maven-artifact-3.8.4.jar:/Users/nyholku/javacpp/jars/maven-core-3.0.jar:/Users/nyholku/javacpp/jars/maven-model-3.8.4.jar:/Users/nyholku/javacpp/jars/maven-plugin-annotations-3.6.2.jar:/Users/nyholku/javacpp/jars/maven-plugin-api-3.8.4.jar:/Users/nyholku/javacpp/jars/osgi.annotation-7.0.0.jar:/Users/nyholku/javacpp/jars/slf4j-simple-1.7.32.jar:/Users/nyholku/javacpp/jars/slf4j-api-1.7.33.jar:/Users/nyholku/javacpp-occ/src occ/TKernel.java 
occ/TKernel.java:555: error: annotation type not applicable to this kind of declaration
        }@Opaque 
         ^
Note: occ/TKernel.java uses unchecked or unsafe operations.

because the generated code looks like:

@Opaque 
                public static native void bar();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does that because it's missing a name for the class, like this works:

infoMap.put(new Info("Foo").annotations("@Opaque").pointerTypes("Foo"));

It's probably a bug, but since it's not a big issue, I've never looked into it.
You're welcome to debug this if you want though.

Copy link
Author

@nyholku nyholku Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That works for the simple test case.

I tried to add it to OCC Standard_Transient and I got errors like this:

Info: javac -cp /Users/nyholku/javacpp/target/classes:/Users/nyholku/javacpp/jars/junit-4.13.2.jar:/Users/nyholku/javacpp/jars/maven-artifact-3.8.4.jar:/Users/nyholku/javacpp/jars/maven-core-3.0.jar:/Users/nyholku/javacpp/jars/maven-model-3.8.4.jar:/Users/nyholku/javacpp/jars/maven-plugin-annotations-3.6.2.jar:/Users/nyholku/javacpp/jars/maven-plugin-api-3.8.4.jar:/Users/nyholku/javacpp/jars/osgi.annotation-7.0.0.jar:/Users/nyholku/javacpp/jars/slf4j-simple-1.7.32.jar:/Users/nyholku/javacpp/jars/slf4j-api-1.7.33.jar:/Users/nyholku/javacpp-occ/src occ/TKernel.java 
occ/TKernel.java:127: error: annotation type not applicable to this kind of declaration
public static native int HashCode(@Const @Opaque Standard_Transient theTransientObject,
                                         ^
occ/TKernel.java:56: error: annotation type not applicable to this kind of declaration
  @Opaque private native void allocate();

for generated code like this:

@Opaque @NoOffset public static class Standard_Transient extends Pointer {
    static { Loader.load(); }
    /** Pointer cast constructor. Invokes {@link Pointer#Pointer(Pointer)}. */
    public Standard_Transient(Pointer p) { super(p); }
    /** Native array allocator. Access with {@link Pointer#position(long)}. */
    public Standard_Transient(long size) { super((Pointer)null); allocateArray(size); }
    private native void allocateArray(long size);
    @Override public Standard_Transient position(long position) {
        return (Standard_Transient)super.position(position);
    }
    @Override public Standard_Transient getPointer(long i) {
        return new Standard_Transient((Pointer)this).offsetAddress(i);
    }


  /** Empty constructor */
  public Standard_Transient() { super((Pointer)null); allocate(); }
  @Opaque private native void allocate();

I guess I could not process the "Standard_Transient.hxx" and just keep it opaque totally, not sure if this is going to leave some essential functionality not accessible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we might need to add a new Info field for that...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about this, and this is somewhat similar to the situation with @NoException. This isn't too useful to use with something like Info.annotation because not having it doesn't harm anything. It just makes function calls a bit slower. When we want to use it because we know the functions are never going to throw exceptions, for example, from libraries written in C like FFmpeg, then we can add it to the presets classes themselves like this here:
https://github.com/bytedeco/javacpp-presets/blob/master/mkl/src/main/java/org/bytedeco/mkl/presets/mkl_rt.java#L59
I'd see something similar in this case, but in reverse, in the sense that we could have every JNI function query the map to perform downcasting if we wanted to, it doesn't harm anything, it just makes everything slower, right? So by default we leave things like they are now, since unlike exceptions it's not something that most libraries need, but if we put the annotation on the presets class, Generator finds it there, and starts doing its thing for all classes and functions covered by those presets. How does that sound?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if you prefer, we could add a new field like Info.dynamize, and I think I'd prefer the corresponding annotation to be @Dynamic, which is clearer than @Downcast, I think. What do you think?

}
}

if (!baseClasses.isEmpty())
outputDowncastFunDefinition();

out.println("static void JavaCPP_dcast_preload_classes(JNIEnv* env) {");
// Create/output runtime cache initialisation code for all target classes that are derived from any of the baseclasses
for (Class c : allClasses) {
// if super class is InfoMapper then this is likely a target class else skip this
if (!InfoMapper.class.isAssignableFrom(c.getSuperclass()))
continue;

for (Class<?> c2 : c.getClasses()) {
if (c2.isInterface())
continue;
// follow up the super class chain to see if this is derived from a baseclass
var sc = c2;
while (sc != null) {
if (baseClasses.contains(sc.getSimpleName()))
break;
sc = sc.getSuperclass();
}
if (sc == null) {
continue;
}

String canonName = c2.getCanonicalName();
int i = canonName.lastIndexOf(".");
String javaName = (canonName.substring(0, i) + "$" + canonName.substring(i + 1)).replaceAll("\\.", "/");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c2.getName().replace('.', '/') should give you the same thing and is a lot simpler.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamize/dynamic is ok, I think your suggestion is good. I will see if I have some time to work on this during the next two week.s

Name na = c2.getAnnotation(Name.class);
String cppTypeName = na != null ? na.value()[0] : c2.getSimpleName();
out.println(" JavaCPP_dcast_preload(env, \"" + javaName + "\", typeid(::" + cppTypeName + "));");
}
}
out.println(" }");

}

public boolean generate(String sourceFilename, String jniConfigFilename, String reflectConfigFilename, String headerFilename,
String loadSuffix, String baseLoadSuffix, String classPath, Class<?> ... classes) throws IOException {
try {
Expand Down Expand Up @@ -1646,6 +1766,7 @@ boolean classes(boolean handleExceptions, boolean defineAdapters, boolean conver
out.println();
out.println("JNIEXPORT jint JNICALL JNI_OnLoad" + baseLoadSuffix + "(JavaVM* vm, void* reserved);");
out.println("JNIEXPORT void JNICALL JNI_OnUnload" + baseLoadSuffix + "(JavaVM* vm, void* reserved);");
out.println("static void JavaCPP_dcast_preload_classes(JNIEnv* env);");
}
out.println(); // XXX: JNI_OnLoad() should ideally be protected by some mutex
out.println("JNIEXPORT jint JNICALL JNI_OnLoad" + loadSuffix + "(JavaVM* vm, void* reserved) {");
Expand Down Expand Up @@ -1691,6 +1812,8 @@ boolean classes(boolean handleExceptions, boolean defineAdapters, boolean conver
out.println(" env->PopLocalFrame(NULL);");
out.println(" }");
out.println(" }");
if (baseLoadSuffix != null && !baseLoadSuffix.isEmpty())
out.println(" JavaCPP_dcast_preload_classes(env);");
out.println(" }");
out.println(" JavaCPP_addressFID = JavaCPP_getFieldID(env, " +
jclasses.index(Pointer.class) + ", \"address\", \"J\");");
Expand Down Expand Up @@ -1862,7 +1985,9 @@ boolean classes(boolean handleExceptions, boolean defineAdapters, boolean conver
out.println(" JavaCPP_vm = NULL;");
out.println("}");
out.println();


outputDowncastFunDeclaration();

boolean supportedPlatform = false;
LinkedHashSet<Class> allClasses = new LinkedHashSet<Class>();
if (baseLoadSuffix == null || baseLoadSuffix.isEmpty()) {
Expand All @@ -1889,6 +2014,9 @@ boolean classes(boolean handleExceptions, boolean defineAdapters, boolean conver

out.println("}");
out.println();

outputDowncastCache(allClasses);

if (out2 != null) {
out2.println("#ifdef __cplusplus");
out2.println("}");
Expand Down Expand Up @@ -2325,7 +2453,10 @@ String returnBefore(MethodInformation methodInfo) {
out.println(" jobject rarg = obj;");
} else if (methodInfo.returnType.isPrimitive()) {
out.println(" " + jniTypeName(methodInfo.returnType) + " rarg = 0;");
returnPrefix = typeName[0] + " rval" + typeName[1] + " = " + cast;
if (typeName.length >= 2)
returnPrefix = typeName[0] + " rval" + typeName[1] + " = " + cast;
else
returnPrefix = typeName[0] + " rval" + " = " + cast;
if ((returnBy instanceof ByPtr) || (returnBy instanceof ByPtrRef)) {
returnPrefix += "*";
}
Expand Down Expand Up @@ -2818,7 +2949,14 @@ void returnAfter(MethodInformation methodInfo) {
}
}
out.println( "if (rptr != NULL) {");
out.println(indent + " rarg = JavaCPP_createPointer(env, " + jclasses.index(methodInfo.returnType) +

Downcast a = (Downcast) methodInfo.method.getAnnotation(Downcast.class);
if (a != null)
out.println(indent + " rarg = JavaCPP_downcast<" + a.base() + ">(env, rptr);");
else
out.println(indent + " rarg = JavaCPP_createPointer(env, " + jclasses.index(methodInfo.returnType) + (methodInfo.parameterTypes.length > 0 && methodInfo.parameterTypes[0] == Class.class ? ", arg0);" : ");"));

out.println(indent + " rarg = JavaCPP_createPointer(env, " + jclasses.index(methodInfo.returnType) +
(methodInfo.parameterTypes.length > 0 && methodInfo.parameterTypes[0] == Class.class ? ", arg0);" : ");"));
out.println(indent + " if (rarg != NULL) {");
if (needInit) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/bytedeco/javacpp/tools/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ Type type(Context context, boolean definition) throws ParserException {
}
type.cppName += separator;
Info info = infoMap.getFirst(t.cppName);
String s = info != null && info.cppTypes != null ? info.cppTypes[0] : t.cppName;
String s = info != null && info.cppTypes != null && info.cppTypes.length>0? info.cppTypes[0] : t.cppName;
if (t.constValue && !s.startsWith("const ")) {
s = "const " + s;
}
Expand Down