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

Primary change is HDFFV-11212 - new refs and JNI #372

Merged
merged 82 commits into from
Feb 25, 2021
Merged

Primary change is HDFFV-11212 - new refs and JNI #372

merged 82 commits into from
Feb 25, 2021

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Feb 23, 2021

Added tests for JNI export dataset and updated export for new refs.
See new src/hdf5lib/H5.java function, src/jni/ h5util.c(.h), test/TestH5.java and test/H5Rref.java.
Updated java hid_t initializations to H5_INVALID_ID instead of -1. All examples and java tests, plus src/jni/h5constants.c.
Removed unused argument to h5str_sprintf function in jni files; h5aimp.c and h5dimp.c.
Tools changes:
Corrected long double printf specifier from %Lf to %Lg in lib/h5tools_str.c.
Close id in lib/h5tools_dump.c.
Correct use of options->fout_fapl in src/h5repack/h5repack_copy.c and src/h5repack_verify.c.

lib/h5diff_array.c, updated latest h5diff subset code - tests remain disabled.

java/src/jni/h5util.c Outdated Show resolved Hide resolved
java/src/jni/h5util.c Outdated Show resolved Hide resolved
@byrnHDF
Copy link
Contributor Author

byrnHDF commented Feb 23, 2021

Updated h5util files with issues discovered.

hsize_t cnt_idx = 0;
hsize_t hs_idx = 0;
j = opts->rank - 1;
hsize_t prev_dim_size = 0; /* previous dim size */
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change about?

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 the code to convert a hyperslab contiguous index to the original index coordinates for the display output.

@@ -76,7 +76,6 @@ check_options(diff_opt_t *opts)
}
}

#if TRILABS_227
/*-------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this macro always defined now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No macro, just a code holder.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that code is active now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, most of it has been in for awhile, but I need to test it more widely.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Were you defining TRILABS_227 by hand then? Or was it in a configure option? Or ???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a way to keep the code for the release and come back to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, there was a refactor of h5diff functions that was triggered by the work and I just needed to merge for the release without losing working code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was confusing (obviously! :-) - maybe put it on a branch next time?

Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

The non-Java changes look good to me.

@@ -0,0 +1,562 @@
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* Copyright by The HDF Group. *
* Copyright by the Board of Trustees of the University of Illinois. *
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a new file? If so, it shouldn't have the line or the Board of Trustees. Also, the support URL 6 lines down should be replaced with " * distribution tree, or in https://www.hdfgroup.org/licenses. *"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a split of TestH5R.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually all these java test files are new since like 2010.

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.

4 participants