search

GrammaticalFramework / gf-core

Grammatical Framework core: compiler, shell & runtimes
https://www.grammaticalframework.org
Other
131 stars 35 forks source link

Java bindings to Majestic runtime #133

Closed harisont closed 1 year ago

harisont commented 3 years ago

Hi, I'm getting started with the Java bindings for the new runtime and so far it seems I got readPGF(path) and getAbstractName working.

Even though this is still very little, I'm opening this pull request so that maybe @krangelov can tell me how I should go about in terms of memory management in what I already implemented, so that I can continue along those lines (I have not noticed any strange behavior so far, but I'd be surprised if there were no memory issues, I don't really have a clue about that, and especially not in the context of the JNI).

krangelov commented 3 years ago

Looks good! There are three things that I noticed:

On Fri, 24 Sept 2021 at 17:02, Arianna Masciolini @.***> wrote:

Hi, I'm getting started with the Java bindings for the new runtime and so far it seems I got readPGF(path) and getAbstractName working.

Even though this is still very little, I'm opening this pull request so that maybe @krangelov https://github.com/krangelov can tell me how I should go about in terms of memory management in what I already implemented, so that I can continue along those lines (I have not noticed any strange behavior so far, but I'd be surprised if there were no memory issues, I don't really have a clue about that, and especially not in the context of the JNI).

You can view, comment on, or merge this pull request online at:

https://github.com/GrammaticalFramework/gf-core/pull/133 Commit Summary

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZG7LP2VFYPUI7NPEGDUDSHIJANCNFSM5EWEY4BQ . 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.

harisont commented 3 years ago

Thank you! I might come back with further question as I make these changes but everything seems clear for now.

harisont commented 3 years ago

I'm not sure how to do the last thing.

I understand why I should use NewString instead of NewStringUTF, but I do not know how to get from let's say txt->text to the corresponding string in the correct encoding.

If I am not mistaken, txt->text is standard UTF-8 to be converted to UTF-16, and I assume conversion is similar to that in gu2j_string. The thing is, I don't really understand how GuStrings and GuUCSs work, which makes me unsure about how a hypothetical utf82j_string should differ from gu2j_string itself.

krangelov commented 3 years ago

Push the code as it is and I can update it.

On Mon, 27 Sept 2021 at 15:48, Arianna Masciolini @.***> wrote:

I'm not sure how to do the last thing.

I understand why I should use NewString instead of NewStringUTF, but I do not know how to get from let's say txt->text to the corresponding string in the correct encoding.

If I am not mistaken, txt->text is standard UTF-8 to be converted to UTF-16, and I assume conversion is similar to that in gu2j_string. The thing is, I don't really understand how GuStrings and GuUCSs work, which makes me unsure about how a hypothetical utf82j_string should differ from gu2j_string itself.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-927892871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZGHJVOPIHHAEJHUPBLUEBY3PANCNFSM5EWEY4BQ . 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.

harisont commented 3 years ago

Question for @krangelov:

I'm working on some abstract syntax methods of the PGF class, specifically getStartCat and getCategories, and looking at the old bindings I noticed that they return respectively a String and a list of List<String>. Given that there is already a class Type as part of the Java bindings, shouldn't these method return instead a Type and a list of Types (like in Haskell and Python)?

To me, that seems only logical, but at the same time I imagine that there is a reason for having done otherwise and wanted to know what your thoughts are before breaking backwards compatibility.

harisont commented 3 years ago

Similarly, shouldn0t the input of getFunctionsByCat be a Type?

krangelov commented 3 years ago

Probably when I added getStartCat, there wasn't a type for classes. Features in the Java binding were developed on demand. I think getStartCat should return Type.

getCategories should return strings just like getFunctions which also returns only function names.

On Thu, 30 Sept 2021 at 15:43, Arianna Masciolini @.***> wrote:

Question for @krangelov https://github.com/krangelov:

I'm working on some abstract syntax methods of the PGF class, specifically getStartCat and getCategories, and looking at the old bindings I noticed that they return respectively a String and a list of List. Given that there is already a class Type as part of the Java bindings, shouldn't these method return instead a Type and a list of Types (like in Haskell and Python)?

To me, that seems only logical, but at the same time I imagine that there is a reason for having done otherwise and wanted to know what your thoughts are before breaking backwards compatibility.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-931335174, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZEBAAWARKFRM3VXEELUERSOLANCNFSM5EWEY4BQ . 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.

krangelov commented 3 years ago

On Thu, 30 Sept 2021 at 16:09, Arianna Masciolini @.***> wrote:

Similarly, shouldn0t the input of getFunctionsByCat be a Type?

No. getFunctionsByCat should take a string as input. If we filter by type then we must do type checking as well.

krangelov commented 3 years ago

What do you mean? I see this in the master repository:

/** Returns the type of the function with the given name.

On Wed, 20 Oct 2021 at 12:37, Arianna Masciolini @.***> wrote:

Probably when I added getStartCat, there wasn't a type for classes. Features in the Java binding were developed on demand. I think getStartCat should return Type. getCategories should return strings just like getFunctions which also returns only function names.

Maybe getStartCat should also return a string after all. getFunctionType also returns a string instead of a Type, now that I look at it...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-947541503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZCCPW3FTLB7NYPKKCDUH2LXBANCNFSM5EWEY4BQ . 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.

harisont commented 3 years ago

Yes, sorry. I had mixed up two things, in fact I deleted the comment immediately, but you still got the email. On Oct 20 2021, at 2:14 pm, Krasimir Angelov @.***> wrote:

What do you mean? I see this in the master repository: /** Returns the type of the function with the given name.

  • @param fun The name of the function. */ public native Type getFunctionType(String fun);

On Wed, 20 Oct 2021 at 12:37, Arianna Masciolini @.***> wrote:

Probably when I added getStartCat, there wasn't a type for classes. Features in the Java binding were developed on demand. I think getStartCat should return Type. getCategories should return strings just like getFunctions which also returns only function names.

Maybe getStartCat should also return a string after all. getFunctionType also returns a string instead of a Type, now that I look at it...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-947541503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZCCPW3FTLB7NYPKKCDUH2LXBANCNFSM5EWEY4BQ . 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. Reply to this email directly, view it on GitHub (https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-947606169), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AEJI3XYLCBJU7442H5MEPM3UH2XDZANCNFSM5EWEY4BQ).

harisont commented 3 years ago

I'm having issues implementing the marshaller. In particular, I'm working on match_type.

I know that this makes the program crash, and initially I thought that the issue was related to casting the result of getObjectField to jobjectArray (or, in general, that it was an array problem), but that doesn't seem to be the case. In fact, I think the reason for the failure is that the fieldID I pass to getObjectField is NULL, even though I don't understand why, and even though I thought that, with a null field id, GetObjectField itself would fail in the first place.

I then also noticed that, similarly, trying to use the cat field also causes the program to crash.

Therefore I think that the problem is not specific to any field of Type objects, and I suspect it is somehow connected to the env pointer, which I don't know if I'm using correctly. To be honest, even if so far everything seems to work in the unmarshaller I'm in general unsure about what I should do with it in both marshalling and unmarshalling.

Can anyone help me with that?

krangelov commented 3 years ago

In both cases, I see that there is a missing semicolon. It should be:

"[Lorg/grammaticalframework/pgf/Hypo;" "Ljava/lang/String;"

I think that is the reason.

In general if you get 0 from GetFieldID then you should return back to Java. Java will then show an exception which will hopefully clarify the issue.

On Tue, 26 Oct 2021 at 15:06, Arianna Masciolini @.***> wrote:

I'm having issues implementing the marshaller. In particular, I'm working on match_type.

I know that this https://github.com/harisont/gf-core/blob/0fdcce39fcc91ed9968e22d5c8a8873f7f9c8837/src/runtime/java/jpgf.c#L161 makes the program crash, and initially I thought that the issue was related to casting the result of getObjectField to jobjectArray https://github.com/harisont/gf-core/blob/0fdcce39fcc91ed9968e22d5c8a8873f7f9c8837/src/runtime/java/jpgf.c#L160 (or, in general, that it was an array problem), but that doesn't seem to be the case. In fact, I think the reason for the failure is that the fieldID I pass to getObjectField https://github.com/harisont/gf-core/blob/0fdcce39fcc91ed9968e22d5c8a8873f7f9c8837/src/runtime/java/jpgf.c#L159 is NULL, even though I don't understand why, and even though I thought that, with a null field id, GetObjectField itself would fail in the first place.

I then also noticed that, similarly, trying to use the cat field also causes the program to crash https://github.com/harisont/gf-core/blob/0fdcce39fcc91ed9968e22d5c8a8873f7f9c8837/src/runtime/java/jpgf.c#L168-L170 .

Therefore I think that the problem is not specific to any field of Type objects, and I suspect it is somehow connected to the env pointer, which I don't know if I'm using correctly. To be honest, even if so far everything seems to work in the unmarshaller I'm in general unsure about what I should do with it in both marshalling and unmarshalling.

Can anyone help me with that?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-951919490, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZADDUKPQ55V3TI4NE3UI2YU5ANCNFSM5EWEY4BQ . 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.

harisont commented 3 years ago

Oh but of course! :facepalm: The example(s) in the JNI documentation had made me misunderstand what the semicolons'role in signatures is

krangelov commented 3 years ago

I also missed the semicolon the first time when I used the JNI. Something in their documentation should be improved.

On Wed, 27 Oct 2021 at 10:07, Arianna Masciolini @.***> wrote:

Oh but of course! 🤦 The example(s) in the JNI documentation had made me misunderstand what the semicolons'role in signatures is

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GrammaticalFramework/gf-core/pull/133#issuecomment-952645851, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYFSZBTZV5E4Z6PDADEXETUI66MJANCNFSM5EWEY4BQ . 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.