luben / zstd-jni

JNI binding for Zstd
Other
809 stars 165 forks source link

Remove dependency on internal headers #222

Closed mkoncek closed 2 years ago

mkoncek commented 2 years ago

This commit completely removes the need to include internal headers and expands error macros. This greatly help us with packaging as now only the public headers are required.

mkoncek commented 2 years ago

@luben I see 3 options: 1) Call into libzstd ZSTD_getErrorCode

mkoncek commented 2 years ago

Unrelated, but I found this:

$ grep -n '[^-]ZSTD_error' *.c
jni_fast_zstd.c:336:        return ZSTD_error_srcSize_wrong;
jni_zstd.c:198:    if (ddict == NULL) return ZSTD_error_dictionary_wrong;
jni_zstd.c:232:    if (cdict == NULL) return ZSTD_error_dictionary_wrong;
jni_zstd.c:363:      return ZSTD_error_##err; \

The first three cases where you don't return the negative error code, is that correct? Everywhere else you do return negative values.

mkoncek commented 2 years ago

This is my suggestion https://github.com/luben/zstd-jni/compare/master...mkoncek:internal-headers2

luben commented 2 years ago

Your suggestion looks good for me. I think there will be warnings if explicit casts are needed.

mkoncek commented 2 years ago

Good, I will double-check if I understand the rules of integer casts correctly and that I am not changing behaviour.

mkoncek commented 2 years ago

Point 2 and 3: https://www.iso-9899.info/n1570.html#6.3.1.3 signed -> unsigned is well defined, unsigned -> signed is implementation-defined. While I was in favor of using only signed types in those functions, ZSTD library functions return size_t whereas JNI bindings often return signed types. Therefore there is no way around implementation-defined behaviour. So my suggested patch is probably the best approach with no further modifications to types used inside JNI functions.

mkoncek commented 2 years ago

I rebased this PR to the suggested one.

luben commented 2 years ago

Merged. Thanks for the contribution!