Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

Commit 1e7858f

Browse files
bzzbzz
committed
address review feedback
 - better error handling - better string literals - doc added Signed-off-by: Alexander Bezzubov <[email protected]>
1 parent 52e3e65 commit 1e7858f

File tree

5 files changed

+109
-70
lines changed

5 files changed

+109
-70
lines changed

‎src/main/native/jni_utils.cc‎

Lines changed: 68 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -20,49 +20,78 @@ JNIEnv *getJNIEnv() {
2020
return pEnv;
2121
}
2222

23-
// Class fully qualified names
24-
const char *CLS_NODE = "org/bblfsh/client/v2/Node";
25-
const char *CLS_CTX = "org/bblfsh/client/v2/Context";
26-
const char *CLS_OBJ = "java/lang/Object";
27-
const char *CLS_RE = "java/lang/RuntimeException";
28-
29-
const char *CLS_JNODE = "org/bblfsh/client/v2/JNode";
30-
const char *CLS_JNULL = "org/bblfsh/client/v2/JNull";
31-
const char *CLS_JSTR = "org/bblfsh/client/v2/JString";
32-
const char *CLS_JINT = "org/bblfsh/client/v2/JInt";
33-
const char *CLS_JFLT = "org/bblfsh/client/v2/JFloat";
34-
const char *CLS_JBOL = "org/bblfsh/client/v2/JBool";
35-
const char *CLS_JUINT = "org/bblfsh/client/v2/JUint";
36-
const char *CLS_JARR = "org/bblfsh/client/v2/JArray";
37-
const char *CLS_JOBJ = "org/bblfsh/client/v2/JObject";
38-
39-
// Method signatures
40-
const char *METHOD_JNODE_KEY_AT = "(I)Ljava/lang/String;";
41-
const char *METHOD_JNODE_VALUE_AT = "(I)Lorg/bblfsh/client/v2/JNode;";
42-
const char *METHOD_JOBJ_ADD =
23+
const char CLS_NODE[] = "org/bblfsh/client/v2/Node";
24+
const char CLS_CTX[] = "org/bblfsh/client/v2/Context";
25+
const char CLS_OBJ[] = "java/lang/Object";
26+
const char CLS_RE[] = "java/lang/RuntimeException";
27+
const char CLS_JNODE[] = "org/bblfsh/client/v2/JNode";
28+
const char CLS_JNULL[] = "org/bblfsh/client/v2/JNull";
29+
const char CLS_JSTR[] = "org/bblfsh/client/v2/JString";
30+
const char CLS_JINT[] = "org/bblfsh/client/v2/JInt";
31+
const char CLS_JFLT[] = "org/bblfsh/client/v2/JFloat";
32+
const char CLS_JBOOL[] = "org/bblfsh/client/v2/JBool";
33+
const char CLS_JUINT[] = "org/bblfsh/client/v2/JUint";
34+
const char CLS_JARR[] = "org/bblfsh/client/v2/JArray";
35+
const char CLS_JOBJ[] = "org/bblfsh/client/v2/JObject";
36+
37+
const char METHOD_JNODE_KEY_AT[] = "(I)Ljava/lang/String;";
38+
const char METHOD_JNODE_VALUE_AT[] = "(I)Lorg/bblfsh/client/v2/JNode;";
39+
const char METHOD_JOBJ_ADD[] =
4340
"(Ljava/lang/String;Lorg/bblfsh/client/v2/JNode;)Lscala/collection/"
4441
"mutable/Buffer;";
45-
const char *METHOD_JARR_ADD =
42+
const char METHOD_JARR_ADD[] =
4643
"(Lorg/bblfsh/client/v2/JNode;)Lscala/collection/mutable/Buffer;";
4744

48-
const char *METHOD_OBJ_TO_STR = "()Ljava/lang/String;";
45+
const char METHOD_OBJ_TO_STR[] = "()Ljava/lang/String;";
4946

47+
// TODO(bzz): cache classes&methods in JNI_OnLoad should speed this up
5048
void checkJvmException(std::string msg) {
5149
JNIEnv *env = getJNIEnv();
52-
if (env->ExceptionCheck()) {
53-
// env->ExceptionDescribe(); // prints to stdout, un-comment for debuging
54-
auto err = env->ExceptionOccurred();
50+
auto err = env->ExceptionOccurred();
51+
if (err) {
52+
env->ExceptionClear();
53+
54+
auto exceptionCls = env->FindClass(CLS_RE);
55+
if (env->ExceptionCheck()) {
56+
env->ExceptionClear();
57+
env->Throw(env->ExceptionOccurred());
58+
return;
59+
}
60+
61+
jclass cls = env->FindClass(CLS_OBJ);
62+
if (env->ExceptionCheck()) {
63+
env->ExceptionClear();
64+
env->ThrowNew(
65+
exceptionCls,
66+
msg.append(" - failed to find class ").append(CLS_OBJ).data());
67+
return;
68+
}
69+
70+
jmethodID toString = env->GetMethodID(cls, "toString", METHOD_OBJ_TO_STR);
71+
if (env->ExceptionCheck()) {
72+
env->ExceptionClear();
73+
env->ThrowNew(exceptionCls,
74+
msg.append(" - failed to find method toString").data());
75+
return;
76+
}
5577

56-
jmethodID toString = env->GetMethodID(env->FindClass(CLS_OBJ), "toString",
57-
METHOD_OBJ_TO_STR);
5878
jstring s = (jstring)env->CallObjectMethod(err, toString);
79+
if (env->ExceptionCheck() || !s) {
80+
env->ThrowNew(exceptionCls,
81+
msg.append(" - failed co call method toString").data());
82+
return;
83+
}
84+
5985
const char *utf = env->GetStringUTFChars(s, 0);
6086
env->ReleaseStringUTFChars(s, utf);
6187

62-
env->ExceptionClear();
88+
// new RuntimeException(msg.data(), err)
89+
jmethodID initId = env->GetMethodID(
90+
cls, "<init>", "(Ljava/lang/String;Ljava/lang/Throwable;)V");
91+
jthrowable exception = (jthrowable)env->NewObject(
92+
exceptionCls, initId, msg.append(": ").append(utf).data(), err);
6393

64-
auto exception = env->FindClass(CLS_RE);
65-
env->ThrowNew(exception, msg.append(": ").append(utf).data());
94+
env->Throw(exception);
6695
}
6796
}
6897

@@ -72,8 +101,10 @@ jobject NewJavaObject(JNIEnv *env, const char *className, const char *initSign,
72101
checkJvmException(std::string("failed to find a class ").append(className));
73102

74103
jmethodID initId = env->GetMethodID(cls, "<init>", initSign);
75-
checkJvmException(
76-
std::string("failed to call <inti> constructor for ").append(className));
104+
checkJvmException(std::string("failed to call a constructor with signature ")
105+
.append(initSign)
106+
.append(" for the class name ")
107+
.append(className));
77108

78109
va_list varargs;
79110
va_start(varargs, initSign);
@@ -87,7 +118,7 @@ jobject NewJavaObject(JNIEnv *env, const char *className, const char *initSign,
87118

88119
jfieldID getField(JNIEnv *env, jobject obj, const char *name) {
89120
jclass cls = env->GetObjectClass(obj);
90-
checkJvmException("failed get an object class");
121+
checkJvmException("failed get the class of an object");
91122

92123
jfieldID jfid = env->GetFieldID(cls, name, "J");
93124
checkJvmException(std::string("failed get a field ").append(name));
@@ -101,7 +132,7 @@ static jmethodID MethodID(JNIEnv *env, const char *method,
101132
checkJvmException(std::string("failed to find a class ").append(className));
102133

103134
jmethodID mId = env->GetMethodID(cls, method, signature);
104-
checkJvmException(std::string("failed get a method ")
135+
checkJvmException(std::string("failed to get method ")
105136
.append(className)
106137
.append(".")
107138
.append(method));
@@ -112,13 +143,13 @@ static jmethodID MethodID(JNIEnv *env, const char *method,
112143
jint IntMethod(JNIEnv *env, const char *method, const char *signature,
113144
const char *className, const jobject *object) {
114145
jmethodID mId = MethodID(env, method, signature, className);
115-
checkJvmException(std::string("failed get a method ")
146+
checkJvmException(std::string("failed to get method ")
116147
.append(className)
117148
.append(".")
118149
.append(method));
119150

120151
jint res = env->CallIntMethod(*object, mId);
121-
checkJvmException(std::string("failed call a method ")
152+
checkJvmException(std::string("failed to call method ")
122153
.append(className)
123154
.append(".")
124155
.append(method)
@@ -131,7 +162,7 @@ jint IntMethod(JNIEnv *env, const char *method, const char *signature,
131162
jobject ObjectMethod(JNIEnv *env, const char *method, const char *signature,
132163
const char *className, const jobject *object, ...) {
133164
jmethodID mId = MethodID(env, method, signature, className);
134-
checkJvmException(std::string("failed get a method ")
165+
checkJvmException(std::string("failed to get method ")
135166
.append(className)
136167
.append(".")
137168
.append(method));

‎src/main/native/jni_utils.h‎

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,28 +4,34 @@
44
#include <jni.h>
55
#include <string>
66

7-
extern const char *CLS_NODE;
8-
extern const char *CLS_CTX;
9-
extern const char *CLS_OBJ;
10-
extern const char *CLS_RE;
11-
12-
extern const char *CLS_JNODE;
13-
extern const char *CLS_JNULL;
14-
extern const char *CLS_JSTR;
15-
extern const char *CLS_JINT;
16-
extern const char *CLS_JFLT;
17-
extern const char *CLS_JBOL;
18-
extern const char *CLS_JUINT;
19-
extern const char *CLS_JARR;
20-
extern const char *CLS_JOBJ;
21-
22-
extern const char *METHOD_JNODE_KEY_AT;
23-
extern const char *METHOD_JNODE_VALUE_AT;
24-
extern const char *METHOD_JOBJ_ADD;
25-
extern const char *METHOD_JARR_ADD;
26-
27-
extern const char *METHOD_OBJ_TO_STR;
28-
7+
// Fully qualified Java class names
8+
extern const char CLS_NODE[];
9+
extern const char CLS_CTX[];
10+
extern const char CLS_OBJ[];
11+
extern const char CLS_RE[];
12+
13+
// Fully qualified class names for Bablefish UAST types
14+
extern const char CLS_JNODE[];
15+
extern const char CLS_JNULL[];
16+
extern const char CLS_JSTR[];
17+
extern const char CLS_JINT[];
18+
extern const char CLS_JFLT[];
19+
extern const char CLS_JBOOL[];
20+
extern const char CLS_JUINT[];
21+
extern const char CLS_JARR[];
22+
extern const char CLS_JOBJ[];
23+
24+
// Method signatures
25+
extern const char METHOD_JNODE_KEY_AT[];
26+
extern const char METHOD_JNODE_VALUE_AT[];
27+
extern const char METHOD_JOBJ_ADD[];
28+
extern const char METHOD_JARR_ADD[];
29+
extern const char METHOD_OBJ_TO_STR[];
30+
31+
// Checks though JNI, if there is a pending excption on JVM side.
32+
//
33+
// Throws new RuntimeExpection to JVM in case there is,
34+
// uses the origial one as a cause and the given string as a message.
2935
void checkJvmException(std::string);
3036

3137
JNIEnv *getJNIEnv();
@@ -38,4 +44,4 @@ jint IntMethod(JNIEnv *, const char *, const char *, const char *,
3844
jobject ObjectMethod(JNIEnv *, const char *, const char *, const char *,
3945
const jobject *, ...);
4046

41-
#endif
47+
#endif

‎src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ class Node : public uast::Node<Node *> {
139139
return NODE_INT;
140140
} else if (env->IsInstanceOf(obj, env->FindClass(CLS_JFLT))) {
141141
return NODE_FLOAT;
142-
} else if (env->IsInstanceOf(obj, env->FindClass(CLS_JBOL))) {
142+
} else if (env->IsInstanceOf(obj, env->FindClass(CLS_JBOOL))) {
143143
return NODE_BOOL;
144144
} else if (env->IsInstanceOf(obj, env->FindClass(CLS_JUINT))) {
145145
return NODE_BOOL;
@@ -378,8 +378,8 @@ class Interface : public uast::NodeCreator<Node *> {
378378
return create(NODE_FLOAT, i);
379379
}
380380
Node *NewBool(bool v) {
381-
jobject i = NewJavaObject(getJNIEnv(), CLS_JBOL, "(Z)V", v);
382-
checkJvmException("failed to create new " + std::string(CLS_JBOL));
381+
jobject i = NewJavaObject(getJNIEnv(), CLS_JBOOL, "(Z)V", v);
382+
checkJvmException("failed to create new " + std::string(CLS_JBOOL));
383383
return create(NODE_BOOL, i);
384384
}
385385
};

‎src/main/scala/org/bblfsh/client/v2/Context.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ case class Context(nativeContext: Long) {
99
@native def encode(n: Node): ByteBuffer
1010
@native def dispose()
1111

12-
@native def filter() // TODO(bzz)
12+
@native def filter()
1313

1414
// TODO(bzz): add loading of the root node, after clarifying when it's needed
1515
// https://github.com/bblfsh/client-python/blob/master/bblfsh/pyuast.cc#L364

‎src/main/scala/org/bblfsh/client/v2/JNode.scala‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,11 @@ import java.io.Serializable
55
import scala.collection.mutable
66

77
/** UAST nodes representation on the JVM side.
8-
*
9-
* Mirrors https://godoc.org/github.com/bblfsh/sdk/uast/nodes
10-
*/
8+
*
9+
* Mirrors https://godoc.org/github.com/bblfsh/sdk/uast/nodes
10+
*/
1111
sealed abstract class JNode {
12+
/* Dynamic dispatch is a convenience to be called from JNI */
1213
def children: Seq[JNode] = this match {
1314
case JObject(l) => l map (_._2)
1415
case JArray(l) => l
@@ -19,8 +20,7 @@ sealed abstract class JNode {
1920
case JObject(l) => l.size
2021
case JArray(l) => l.size
2122
case JString(l) => l.length
22-
case JNothing => 0
23-
case _ => 1
23+
case _ => 0
2424
}
2525

2626
def keyAt(i: Int): String = this match {
@@ -45,7 +45,9 @@ case object JNothing extends JNode // 'zero' value for JNode
4545
case class JNull() extends JNode
4646
case class JString(str: String) extends JNode
4747
case class JFloat(num: Double) extends JNode
48-
case class JUint(num: Long) extends JNode
48+
case class JUint(num: Long) extends JNode {
49+
def get(): Long = java.lang.Integer.toUnsignedLong(num.toInt)
50+
}
4951
case class JInt(num: Long) extends JNode
5052
case class JBool(value: Boolean) extends JNode
5153

0 commit comments

Comments
 (0)