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

requested changes to java.c, configure.ac, and internal state to cobl… #150

Closed
wants to merge 9 commits into from

Conversation

xevor11
Copy link

@xevor11 xevor11 commented Jun 12, 2024

Rebased and Made requested changes in PR #149

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

please add an entry to DEPENDENCIES about the new optional dependency, and an entry to ChangeLog and libcob/ChangeLog; this will also make it easier to understand what you added why

Please just update your source branch, currently jni-correct and those commits will land in here.

configure.ac Outdated

AC_PATH_PROG(javahome,java, "/usr/bin")
javahome=`dirname ${javahome}`
javahome=`dirname ${javahome}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why twice?
Do we need this for anything later on?

Copy link
Author

Choose a reason for hiding this comment

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

That was by accident, however I was slightly confused about the formatting

configure.ac Outdated
Comment on lines 500 to 503
AC_CHECK_HEADERS([jni.h], [],
[AC_MSG_ERROR([jni.h is required but could not be found])])
AC_CHECK_HEADERS([jni_md.h], [],
[AC_MSG_ERROR([jni_md.h is required but could not be found])])
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can leave this as-is for now, later on it should only be done if --with-java is specified.
You may opt to do this part after your mid-term evaluation, whenever you do check how --with-xml2 is handled.

Comment on lines +22 to +26
lib_LTLIBRARIES = libcob.la
libcob_la_SOURCES = common.c move.c numeric.c strings.c \
fileio.c call.c intrinsic.c termio.c screenio.c reportio.c cobgetopt.c \
java.c \
mlio.c coblocal.h cconv.c system.def profiling.c
Copy link
Collaborator

@GitMensch GitMensch Jun 16, 2024

Choose a reason for hiding this comment

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

you likely just wanted to add java.c to the list below; later on you may include it conditional on a variable (similar but not identical to libodbci).

In any case that's an adjustment, so please add ,2024 to the copyright year of this file

@@ -1,6 +1,6 @@
/*
Copyright (C) 2007-2012, 2014-2022 Free Software Foundation, Inc.
Written by Roger While, Simon Sobisch, Ron Norman
Copyright (C) 2007-2012, 2014-2024 Free Software Foundation, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't just include year numbers, so until @ddeclerck has reached a state where he considers to have many important commit merges done (or you miss a specific 3.x commit that he already merged) and we rebase the java-interop branch which likely will add 2023 commits, the change should say ,2024`.

Comment on lines 452 to 474
COB_HIDDEN char* cob_get_strerror (void);

COB_HIDDEN JNIEnv* cob_create_vm ();
COB_HIDDEN void cob_handle_error (JavaVM* jvm, char* methodSig);
COB_HIDDEN char* cob_gen_method_sig (const char** paramType, int paramCount, const char** returnType);

COB_HIDDEN void cob_lookup_static_method (JNIEnv* env, JavaVM* jvm, const char *className, const char *methodName,
const char *methodSig, const char *returnType, const char** paramTypes, int paramCount);

COB_HIDDEN void cob_static_method (JNIEnv* env, JavaVM* jvm, jclass cls, jmethodID mid);
COB_HIDDEN void cob_call_java_static_method (JNIEnv *env, JavaVM *jvm, const char *className, const char *methodName, jobject obj, jstring input);

enum cob_case_modifier {
CCM_NONE,
CCM_LOWER,
CCM_UPPER,
CCM_LOWER_LOCALE,
CCM_UPPER_LOCALE
};
COB_HIDDEN unsigned char cob_toupper (const unsigned char);
COB_HIDDEN unsigned char cob_tolower (const unsigned char);
COB_HIDDEN int cob_field_to_string (const cob_field *, void *,
const size_t, const enum cob_case_modifier target_case);
Copy link
Collaborator

Choose a reason for hiding this comment

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

something has gone wrong here - see the same content directly above

Copy link
Author

Choose a reason for hiding this comment

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

I do not see any duplicates in the file?

Copy link
Collaborator

@GitMensch GitMensch Jun 17, 2024

Choose a reason for hiding this comment

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

cob_field_to_string is duplicate, cob_tolower/cob_toupper/cob_get_strerror/cob_case_modifier are shown to be added while not related to this PR

Copy link
Author

Choose a reason for hiding this comment

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

I might have changed the line number causing get strerror and the enum to appear in the PR

@xevor11
Copy link
Author

xevor11 commented Jun 17, 2024

Added the requested changes

@GitMensch
Copy link
Collaborator

GitMensch commented Jun 17, 2024

Do you have a first working call to Java (which likely includes and adjustment to call.c as well), maybe System.getProperty("java.version") which is then DISPLAYed by COBOL?

@xevor11
Copy link
Author

xevor11 commented Jul 1, 2024

More changes for the following:

Integrated JNI with java.c by adding types and data structures in java.c for bidirectional communication

Working towards:

Do you have a first working call to Java (which likely includes and adjustment to call.c as well), maybe System.getProperty("java.version") which is then DISPLAYed by COBOL?

jclass cls;
jmethodID mid;
} MethodCache;
#define HAVE_JNI
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be defined by the configure script, but I guess this is a workaround until it is fully operative.

@@ -146,6 +146,8 @@ static struct literal_list *literal_cache = NULL;
static struct field_list *field_cache = NULL;
static struct field_list *local_field_cache = NULL;
static struct call_list *call_cache = NULL;
extern JavaVM *jvm;
extern JNIEnv *env;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure these are needed in the compiler (cobc) itself.

static void lookup_java_call(const char *p) {
struct call_list *clp;

for (clp = call_cache; clp; clp = clp->next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's another list you need to manipulate in this function. Something like call_java_cache.

@xevor11
Copy link
Author

xevor11 commented Jul 10, 2024

@GitMensch Here are the final changes, we fixed issues with autoconf, config and when make it runs without errors. We need to write some tests, but after that we can merge

@nberth
Copy link
Contributor

nberth commented Jul 10, 2024

@GitMensch Here are the final changes, we fixed issues with autoconf, config and when make it runs without errors. We need to write some tests, but after that we can merge

I guess you want to check #157 instead. This PR (#150) is outdated and can probably be closed.

@GitMensch
Copy link
Collaborator

closed in favor of duplicate #157

@GitMensch GitMensch closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants