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.

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

Non-type template with std::enable_if #491

Open
egolearner opened this issue Jun 3, 2021 · 16 comments
Open

Non-type template with std::enable_if #491

egolearner opened this issue Jun 3, 2021 · 16 comments

Comments

@egolearner
Copy link
Contributor

egolearner commented Jun 3, 2021

Our cpp code looks like this

  enum Types {
    FLOAT = 1,
    DOUBLE = 2,
    ...
  }

  template <Types T,
            typename = typename std::enable_if<T == FLOAT>::type>
  int search(const float *vec, size_t dim) {
    ...
  }

  template <Types T,
            typename = typename std::enable_if<T == DOUBLE>::type>
  int search(const double *vec, size_t dim) {
    ...
  }
        infoMap.put(new Info("Someclass::search<FLOAT>").javaNames("search"));

Javacpp will instantiate all the search functions instead of only the FLOAT one. (Subtle to us as we need all the functions.) The bigger problem is generated JNI code will call the function without template argument, leading to compiling error.

    int rval = ptr->search((const float*)ptr0, (size_t)arg1);

Relevant compiling message

candidate template ignored: couldn't infer template argument 'T'
  int search(const float *vec, size_t dim);

How to help JavaCPP generating the right calling code?

@saudet
Copy link
Member

saudet commented Jun 3, 2021

That should work. Could you prepare a small self-contained example that I can try here?

@egolearner
Copy link
Contributor Author

egolearner commented Jun 3, 2021

I can reproduce it with this demo.

#include <iostream>
    enum Types {
        FLOAT = 1,
        DOUBLE = 2,
    };

class TestTemplate {
public:
    template <Types T,
             typename = typename std::enable_if<T == FLOAT>::type>
                 int search(const float *vec, size_t dim) {
                     std::cout << T  << std::endl;
                 }

    template <Types T,
             typename = typename std::enable_if<T == DOUBLE>::type>
                 int search(const double *vec, size_t dim) {
                     std::cout << T  << std::endl;
                 }
};
    @Override
    public void map(InfoMap infoMap) {
        infoMap.put(new Info("TestTemplate::search<FLOAT>").javaNames("search"));
    }

generated java class

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

    public native int search(@Const FloatPointer vec, @Cast("size_t") long dim);
    public native int search(@Const FloatBuffer vec, @Cast("size_t") long dim);
    public native int search(@Const float[] vec, @Cast("size_t") long dim);

    public native int search(@Const DoublePointer vec, @Cast("size_t") long dim);
    public native int search(@Const DoubleBuffer vec, @Cast("size_t") long dim);
    public native int search(@Const double[] vec, @Cast("size_t") long dim);
}

generated jni code

    TestTemplate* ptr = (TestTemplate*)jlong_to_ptr(env->GetLongField(obj, JavaCPP_addressFID));
    if (ptr == NULL) {
        env->ThrowNew(JavaCPP_getClass(env, 7), "This pointer address is NULL.");
        return 0;
    }
    jlong position = env->GetLongField(obj, JavaCPP_positionFID);
    ptr += position;
    float* ptr0 = arg0 == NULL ? NULL : (float*)jlong_to_ptr(env->GetLongField(arg0, JavaCPP_addressFID));
    jlong position0 = arg0 == NULL ? 0 : env->GetLongField(arg0, JavaCPP_positionFID);
    ptr0 += position0;
    jint rarg = 0;
    int rval = ptr->search((const float*)ptr0, (size_t)arg1);
    rarg = (jint)rval;
    return rarg;

PTAL @saudet

saudet added a commit that referenced this issue Jun 4, 2021
…ng parameters as well (issue #492)

 * Fix `Parser` producing incorrect calls to function templates with non-type parameters (issue #491)
@saudet
Copy link
Member

saudet commented Jun 4, 2021

I've fixed the issue regarding the missing template argument in commit b4cf59a, but JavaCPP isn't able to process std::enable_if, so it cannot know which instance is valid or not. That needs to be done manually.

@egolearner
Copy link
Contributor Author

I've fixed the issue regarding the missing template argument in commit b4cf59a, but JavaCPP isn't able to process std::enable_if, so it cannot know which instance is valid or not. That needs to be done manually.

Could you elaborate on how to be done manually?

@saudet
Copy link
Member

saudet commented Jun 4, 2021

I'm thinking that specifying the method parameters as per issue #492 should work?

@egolearner
Copy link
Contributor Author

egolearner commented Jun 4, 2021

I see kind of regression in commit b4cf59a when it comes to universal template with the following compiling message.

candidate function template not viable: no known conversion from 'jdouble' (aka 'double')
      to 'double &&' for 2nd argument
  void set(const std::string &key, T &&val) {

cpp interface looks like this

  template <typename T>
  void set(const std::string &key, T &&val) {}

There is an easy work around.

        infoMap.put(new Info("Someclass::set<float>").javaNames("set").cppNames("set"));

@egolearner
Copy link
Contributor Author

I'm thinking that specifying the method parameters as per issue #492 should work?

Not really.

I tried the following.

        infoMap.put(new Info("TestTemplate::search<DOUBLE>").javaNames("searchDouble"));

Parser generated all the methods leading to jni code compiling error due to std::enable_if.

    public native @Name("search<DOUBLE>") int searchDouble(@Const float[] vec, @Cast("size_t") long dim);
    public native @Name("search<DOUBLE>") int searchDouble(@Const double[] vec, @Cast("size_t") long dim);
        infoMap.put(new Info("TestTemplate::search<FLOAT>(const float*, size_t)").javaNames("searchFloat"));
        infoMap.put(new Info("TestTemplate::search<FLOAT>(const double*, size_t)").javaNames("searchDouble"));

Parser does not emit any search function.

@saudet
Copy link
Member

saudet commented Jun 4, 2021

I see kind of regression in commit b4cf59a when it comes to universal template with the following compiling message.

I cannot reproduce this, it works fine here. As usual, you will need to provide me with a self-contained example that actually fails.

Parser does not emit any search function.

Right, that probably needs to be enhanced a bit more. Info.javaText without method parameters should work though.

@egolearner
Copy link
Contributor Author

I cannot reproduce this, it works fine here. As usual, you will need to provide me with a self-contained example that actually fails.

I made a mistake. It do works.
After change Info from

        infoMap.put(new Info("Univ::set<int>").javaNames("set"));

to

        infoMap.put(new Info("Univ::set<int&&>").javaNames("set"));

for the following demo

class Univ {
    public:
        template <typename T>
            void set(T&&) {}
};

@egolearner
Copy link
Contributor Author

Info.javaText without method parameters should work though.
Parser generated all the methods leading to jni code compiling error due to std::enable_if.

        infoMap.put(new Info("TestTemplate::search<DOUBLE>").javaNames("searchDouble"));
    public native @Name("search<DOUBLE>") int searchDouble(@Const float[] vec, @Cast("size_t") long dim);
    public native @Name("search<DOUBLE>") int searchDouble(@Const double[] vec, @Cast("size_t") long dim);

saudet added a commit that referenced this issue Jun 8, 2021
@saudet
Copy link
Member

saudet commented Jun 8, 2021

        infoMap.put(new Info("TestTemplate::search<FLOAT>(const float*, size_t)").javaNames("searchFloat"));
        infoMap.put(new Info("TestTemplate::search<FLOAT>(const double*, size_t)").javaNames("searchDouble"));

Parser does not emit any search function.

That now works. I've added support for that in commit b7dbf1c. Enjoy!

@egolearner
Copy link
Contributor Author

@saudet Thanks for your great work.
It do works with the demo above. However, it does not work with our production code which is more complicated.
Here is another demo more similar to our code. Both template parameter and function parameter may contain ::.

#include <iostream>
namespace test {
    enum Types {
        FLOAT = 1,
        DOUBLE = 2,
    };

    struct Void {};

}

class TestTemplate {
    public:
        template <test::Types T,
                 typename = typename std::enable_if<T == test::FLOAT>::type>
                     int search(const float *vec, size_t dim, test::Void v) {
                         std::cout << T  << std::endl;
                     }

        template <test::Types T,
                 typename = typename std::enable_if<T == test::FLOAT>::type>
                     int search(const float *vec, size_t dim) {
                         std::cout << T  << std::endl;
                     }

        template <test::Types T,
                 typename = typename std::enable_if<T == test::DOUBLE>::type>
                     int search(const double *vec, size_t dim) {
                         std::cout << T  << std::endl;
                     }
};
    @Override
    public void map(InfoMap infoMap) {
        infoMap.put(new Info("TestTemplate::search<test::FLOAT>(const float*, size_t)").javaNames("searchFloat"));
        infoMap.put(new Info("TestTemplate::search<test::FLOAT>(const float*, size_t, test::Void)").javaNames("searchFloatVoid"));
    }

It seems the problem is with InfoMap normalization logic as C++ is really complicated. The code below try to untemplate by locating template after lastColon while lastColon may point to location after the template parameter.

} else if (untemplate) {
int count = 0, lastColon = -1, template = -1, parameters = -1;
for (int i = 0; i < n; i++) {
if (tokens[i].match('<')) {
count++;
} else if (tokens[i].match('>')) {
count--;
}
if (count == 0 && tokens[i].match("::")) {
lastColon = i;
}
}
for (int i = 0; i < n; i++) {
if (i > lastColon && tokens[i].match('<')) {
if (count == 0) {
template = i;
}
count++;
} else if (i > lastColon && tokens[i].match('>')) {
count--;
if (count == 0) {
parameters = i + 1;
}
}
}
if (template >= 0) {
name = foundConst ? "const " : "";
for (int i = 0; i < template; i++) {
name += tokens[i];
}
for (int i = parameters; i < n; i++) {
name += tokens[i].spacing + tokens[i];
}

@saudet
Copy link
Member

saudet commented Jun 8, 2021

I see, please try again with this patch:

diff --git a/src/main/java/org/bytedeco/javacpp/tools/InfoMap.java b/src/main/java/org/bytedeco/javacpp/tools/InfoMap.java
index d889a167..54b83f9d 100644
--- a/src/main/java/org/bytedeco/javacpp/tools/InfoMap.java
+++ b/src/main/java/org/bytedeco/javacpp/tools/InfoMap.java
@@ -209,7 +209,7 @@ public class InfoMap extends HashMap<String,List<Info>> {
                 name += " " + tokens[i].value;
             }
         } else if (untemplate) {
-            int count = 0, lastColon = -1, template = -1, parameters = -1;
+            int count = 0, lastColon = -1, template = -1, parameters = n;
             for (int i = 0; i < n; i++) {
                 if (tokens[i].match('<')) {
                     count++;
@@ -218,9 +218,12 @@ public class InfoMap extends HashMap<String,List<Info>> {
                 }
                 if (count == 0 && tokens[i].match("::")) {
                     lastColon = i;
+                } else if (count == 0 && tokens[i].match('(')) {
+                    parameters = i;
+                    break;
                 }
             }
-            for (int i = 0; i < n; i++) {
+            for (int i = 0; i < parameters; i++) {
                 if (i > lastColon && tokens[i].match('<')) {
                     if (count == 0) {
                         template = i;
@@ -228,8 +231,8 @@ public class InfoMap extends HashMap<String,List<Info>> {
                     count++;
                 } else if (i > lastColon && tokens[i].match('>')) {
                     count--;
-                    if (count == 0) {
-                        parameters = i + 1;
+                    if (count == 0 && i + 1 != parameters) {
+                        template = -1;
                     }
                 }
             }

@egolearner
Copy link
Contributor Author

With your patch, Name annotation is missing

        public native int searchFloatVoid(@Const FloatPointer vec, @Cast("size_t") long dim, @ByVal Void v);
        public native int searchFloatVoid(@Const FloatBuffer vec, @Cast("size_t") long dim, @ByVal Void v);
        public native int searchFloatVoid(@Const float[] vec, @Cast("size_t") long dim, @ByVal Void v);

        public native int searchFloat(@Const FloatPointer vec, @Cast("size_t") long dim);
        public native int searchFloat(@Const FloatBuffer vec, @Cast("size_t") long dim);
        public native int searchFloat(@Const float[] vec, @Cast("size_t") long dim);

error message

error: no member named 'searchFloat' in 'TestTemplate'
    int rval = ptr->searchFloat((const float*)ptr0, (size_t)arg1);

no member named 'searchFloatVoid' in 'TestTemplate'
    int rval = ptr->searchFloatVoid((const float*)ptr0, (size_t)arg1, *ptr2);

@saudet
Copy link
Member

saudet commented Jun 8, 2021

Ok, that's a separate issue. Please try again with this additional patch:

diff --git a/src/main/java/org/bytedeco/javacpp/tools/Parser.java b/src/main/java/org/bytedeco/javacpp/tools/Parser.java
index 5b6c7abd..41dc2e34 100644
--- a/src/main/java/org/bytedeco/javacpp/tools/Parser.java
+++ b/src/main/java/org/bytedeco/javacpp/tools/Parser.java
@@ -1606,7 +1606,9 @@ public class Parser {
             if (context.namespace != null && localName.startsWith(context.namespace + "::")) {
                 localName = dcl.cppName.substring(context.namespace.length() + 2);
             }
-            if (!localName.equals(dcl.javaName) && (!localName.contains("::") || context.javaName == null)) {
+            int template = localName.lastIndexOf('<');
+            String simpleName = template >= 0 ? localName.substring(0, template) : localName;
+            if (!localName.equals(dcl.javaName) && (!simpleName.contains("::") || context.javaName == null)) {
                 type.annotations += "@Name(\"" + localName + "\") ";
             }
         }
@@ -2360,7 +2362,9 @@ public class Parser {
             if (fullInfo != null && fullInfo.javaNames != null && fullInfo.javaNames.length > 0) {
                 dcl.javaName = fullInfo.javaNames[0];
                 dcl.signature = dcl.javaName + dcl.parameters.signature;
-                if (!localName2.equals(dcl.javaName) && (!localName2.contains("::") || context.javaName == null)) {
+                int template = localName2.lastIndexOf('<');
+                String simpleName = template >= 0 ? localName2.substring(0, template) : localName2;
+                if (!localName2.equals(dcl.javaName) && (!simpleName.contains("::") || context.javaName == null)) {
                     type.annotations = type.annotations.replaceAll("@Name\\(.*\\) ", "");
                     type.annotations += "@Name(\"" + localName2 + "\") ";
                 }

@egolearner
Copy link
Contributor Author

Thanks @saudet
It works!!!

saudet added a commit that referenced this issue Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants