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

Fix names of overloaded methods with ref type parameters #139

Merged
merged 5 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ lazy val plugin = project
.settings(
name := "sbt-jni",
libraryDependencies += "org.ow2.asm" % "asm" % "9.4",
libraryDependencies ++= Seq("org.scalactic" %% "scalactic" % "3.2.15" % Test,
"org.scalatest" %% "scalatest" % "3.2.15" % Test,
"org.scalatest" %% "scalatest-funspec" % "3.2.15" % Test),
// make project settings available to source
Compile / sourceGenerators += Def.task {
val src = s"""|/* Generated by sbt */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ private NativeMethod(int access, String name, Type type, String arguments) {
this.name = name;
this.type = type;
this.mangledName = mangleName(name);
this.longMangledName = mangledName + "__" + mangleName(arguments);
this.longMangledName = mangledName + "__" + mangleArguments(arguments);
}

@Override
Expand Down
48 changes: 36 additions & 12 deletions plugin/src/main/java/com/github/sbt/jni/javah/util/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,53 @@ public void close() throws IOException {
}
});

private static void addChar(char ch, StringBuilder builder) {
if (ch == '.') {
builder.append('_');
} else if (ch == '_') {
builder.append("_1");
} else if (ch == ';') {
builder.append("_2");
} else if (ch == '[') {
builder.append("_3");
} else if ((ch >= '0' && ch <= '9') || (ch >= 'a' && ch <= 'z') || (ch >= 'A' && (ch <= 'Z'))) {
builder.append(ch);
} else {
builder.append(String.format("_0%04x", (int) ch));
}
}

public static String mangleName(String name) {
StringBuilder builder = new StringBuilder(name.length() * 2);
int len = name.length();
for (int i = 0; i < len; i++) {
char ch = name.charAt(i);
if (ch == '.') {
addChar(name.charAt(i), builder);
}
return builder.toString();
}

public static String mangleArguments(String arguments) {
Copy link
Member

Choose a reason for hiding this comment

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

I need to take a closer look and will appreciate some READMEs about the apporach taken with these tasty files, that's quite interesting.

I also checked that this function is backwards compatible on the https://github.com/Glavo/gjavah project tests.
But we still need to ensure sbt-tests of this project are running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not clear on what you are getting at with your mention of READMEs. Did that line belong to the previous comment?

Copy link
Member

Choose a reason for hiding this comment

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

I mean that if we want to keep these tasty files in the repository we need a direct description of what they are, how to use them to debug, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this stuff is gone. You can just compile the source in the test code to see it now.

boolean inObjectType = false;
StringBuilder builder = new StringBuilder(arguments.length() * 2);
int len = arguments.length();
for (int i = 0; i < len; i++) {
char ch = arguments.charAt(i);
if (inObjectType && ch == ';') {
addChar(ch, builder);
inObjectType = false;
} else if (inObjectType && ch == '/') {
builder.append('_');
} else if (ch == '_') {
builder.append("_1");
} else if (ch == ';') {
builder.append("_2");
} else if (ch == '[') {
builder.append("_3");
} else if ((ch >= '0' && ch <= '9') || (ch >= 'a' && ch <= 'z') || (ch >= 'A' && (ch <= 'Z'))) {
builder.append(ch);
} else if (!inObjectType && ch == 'L') {
inObjectType = true;
addChar(ch, builder);
} else {
builder.append(String.format("_0%04x", (int) ch));
addChar(ch, builder);
}
}
return builder.toString();
}

public static String escape(String unicode) {
public static String escape(String unicode) {
Objects.requireNonNull(unicode);
int len = unicode.length();
StringBuilder builder = new StringBuilder(len);
Expand Down
1 change: 1 addition & 0 deletions plugin/src/main/scala/com/github/sbt/jni/build/CMake.scala
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ object CMake extends BuildTool with ConfigureMakeInstall {
s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}",
"-DCMAKE_BUILD_TYPE=Release",
"-DSBT:BOOLEAN=true",
s"-DJAVA_HOME=${scala.sys.props.get("java.home").get}",
Copy link
Member

Choose a reason for hiding this comment

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

What happens in case java.home for some unpredictable reason is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know the answer to that question. I know it did not work on my laptop without providing a java home. The CMake find JNI functionality was lost without it. I think the real solution is to allow this plugin to be configurable to run as is (no JAVA_HOME set), as I modified it, or with a user-specified JAVA_HOME. That should be another PR. I just did enough to get things to work on my laptop.

Copy link
Member

@pomadchin pomadchin Feb 22, 2023

Choose a reason for hiding this comment

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

My comment was that doing .get is unsafe. We can append it to the list of params in case it's present, why not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is modified in the update. Is there a way to make it cleaner?

cmakeVersion.toString,
baseDirectory.getAbsolutePath
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import org.objectweb.asm.{ClassReader, ClassVisitor, MethodVisitor, Opcodes}

object BytecodeUtil {

private class NativeFinder extends ClassVisitor(Opcodes.ASM5) {
private class NativeFinder extends ClassVisitor(Opcodes.ASM9) {

// classes found to contain at least one @native def
val _nativeClasses = new HashSet[String]
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
1 change: 1 addition & 0 deletions plugin/src/test/resources/include/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This directory contains a header file foe the Overloads class that was generated by the javah application of a Java 8 SDK.
Copy link
Member

Choose a reason for hiding this comment

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

I think this does not belong to the resources folder as well as all the related tasty files. But we cat still keep tasty files in a separate folder i.e. in the plugin sbt-test directory in a new directory with a full sample-overloaded project; similar to what we already have.

And we'd need a README to those tasty files and how to deal with them / why we need them.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions plugin/src/test/resources/scala/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This source directory contains source code for producing the class files used in unit tests.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.github.sbt.jni.samples

private[samples] case class A(x: Int)
private[samples] case class B(x: Double)
private[samples] case class C(x: Long)

class Overloads {
@native
def doSomething(param: A): B
@native
def doSomething(param: B): A
@native
def doSomething(param: Array[C]): A
@native
def doSomething(parma0: Int, param1: Array[C], param2:B, param3: Double): A
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a full test, similar to others for this proxy along with its JNI component into a separate project into the sbt-test/sbt-jni directory for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next update

Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.github.sbt.jni.javah

import com.github.sbt.jni.javah.util.ClassMetaInfo
import org.objectweb.asm.ClassReader
import org.scalatest.funspec.AnyFunSpec
import scala.jdk.CollectionConverters.asScalaBuffer

import java.io.File
import java.nio.file.Files

class NativeMethodTests extends AnyFunSpec {
this.
describe("NativeMethods") {
describe("when a method is overloaded") {
it("should name methods correctly") {
val expectedNames = Set("doSomething__Lcom_github_sbt_jni_samples_A_2",
"doSomething__Lcom_github_sbt_jni_samples_B_2",
"doSomething___3Lcom_github_sbt_jni_samples_C_2",
"doSomething__I_3Lcom_github_sbt_jni_samples_C_2Lcom_github_sbt_jni_samples_B_2D")
val methods = asScalaBuffer(NativeMethodTests.classMeta.methods)
for (m <- methods) {
assert(expectedNames.contains(m.longMangledName()))
}
}
}
}
}

object NativeMethodTests {
lazy val classMeta: ClassMetaInfo = {
val resource = getClass.getClassLoader.getResource("classes/com/github/sbt/jni/samples/Overloads.class").getPath
val f = new File(resource)
val meta = new ClassMetaInfo()
val stream = Files.newInputStream(f.toPath)
try {
val reader = new ClassReader(stream)
reader.accept(meta, ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES)
}
finally {
stream.close()
}
meta
}
}