Refactor JNI translate functions to a recursive switch on datatype #2232
Conversation
| if (nelmts < 0) | ||
| H5_BAD_ARGUMENT_ERROR(ENVONLY, "translate_rbuf: number of VL elements < 0"); | ||
|
|
||
| ret_buflen = ENVPTR->GetArrayLength(ENVONLY, ret_buf); |
There was a problem hiding this comment.
No need to do this multiple times for each loop.
There was a problem hiding this comment.
Right - I missed that.
There was a problem hiding this comment.
But then again if ret_buf is only partially inited, I should get the existing members before we create new ones?
There was a problem hiding this comment.
Will move the GetArrayLength call out of the loop.
| ret_buflen = ENVPTR->GetArrayLength(ENVONLY, ret_buf); | ||
| /* The list we're going to return: */ | ||
| if (i < ret_buflen) { | ||
| jList = ENVPTR->GetObjectArrayElement(ENVONLY, (jobjectArray)ret_buf, (jsize)i); |
There was a problem hiding this comment.
Instead, I'd just check if i >= ret_buflen and stop the loop. Otherwise it looks like there's a chance we could pass a NULL jList to translate_rbuf when i >= ret_buflen and probably cause a crash if that ever happens.
Beyond that though, should i ever be >= ret_buflen?
There was a problem hiding this comment.
The idea was that if ret_buf is only initialized (new List() has no members) then we need to create a new member, jList.
There was a problem hiding this comment.
GetObjectArrayElement will error if we try to get a member that doesn't exist.
| switch (typeSize) { | ||
| case sizeof(jbyte): { | ||
| jbyte byteValue; | ||
| for (x = 0; x < typeSize; x++) { |
There was a problem hiding this comment.
These loops probably should be changed to memcpy, maybe?
There was a problem hiding this comment.
Yes, I think it would be more appropriate to just do a memcpy of size sizeof(jbyte). It's still dangerous if typeSize isn't correct, but the loops below are somewhat misleading and may not necessarily be correct.
|
@byrnHDF Mac test failed. Revert this PR. |
|
This PR cannot cause that failure because it only deals with the JNI. That test failing only uses the C Library! |
|
I know but your PR triggered it, didn't it? |
|
the accum test is broken on macOS 12 |
|
I know. |
macos-latest in the workflows/main.yaml file resolved to macOS 11 previously but to macOS 12 today. PR #2260 changes macos-latest to macos-11 to allow github macOS tests to pass the SWMR tests until the issue is fixed. See HDFGroup#2261 and test/accum.c CTest fails on macOS Monterey · Issue #1289 · HDFGroup/hdf5 (github.com). |
Refactored into a switch on datatype class.
Added array implementation and tests for datasets and attributes. (Will add more tests for other combos later)
Still need to implement compounds.
Requires use of H5.H5DwriteVL and H5.H5DreadVL because of existing implementation of non-VL functions in H5.java file.
This implementation of read/write should be able to supersede the others once tested.