Feature/autodowncast4

[nyholku]

I decided to create a new branch and manually re-applied my changes for cleaner history

[nyholku]

Done.

…

On 10. Feb 2022, at 9.51, Samuel Audet ***@***.***> wrote: @saudet commented on this pull request. In src/main/java/org/bytedeco/javacpp/tools/Generator.java <#554 (comment)>: > + 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>(); Please limit yourself to features available in Java SE 7. Syntactic sugar like this isn't worth losing backward compatibility. — Reply to this email directly, view it on GitHub <#554 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFP7LPN3LNOJ4LAXPCYFZ3U2NVAJANCNFSM5N4SB2NQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.

[saudet]

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.

[nyholku]

Ok, how do we go about that?

[saudet]

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

[saudet]

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?

[nyholku]

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.

[saudet]

@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.

[nyholku]

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();

[saudet]

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.

[nyholku]

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.

[saudet]

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

[saudet]

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?

[saudet]

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?

[saudet]

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

[nyholku]

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

[nyholku]

After having had time to review where we are I'm not sure where we are.

What is the consensus what we want to do here?

You want get rid of this logic:

https://github.com/nyholku/javacpp/blob/a570b774babf0c3030b87a555f30e232a19c1a22/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L256-L259

And suggested fix is that we add a @dynamic addition to the base class?

[saudet]

The idea is to have the base class be @Dynamic. We're still going to need to look that up with this kind of code, so that part doesn't change.