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

Show better error message, when the src/dst offset is negative #312

Closed
wants to merge 1 commit into from

Conversation

gzsombor
Copy link
Contributor

No description provided.

Copy link
Owner

@luben luben left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution. Unfortunately I see a problem here: you modify src/main/native/common/error_private.c that comes from upstream (https://github.com/facebook/zstd). I would prefer not to do any changes on these files as they will incur maintenance overhead on each new upstream version - I have not to forget to apply these changes. Also the private files don't have any stability guarantee so they may go away or get refactored making similar change in the future more complicated

@gzsombor
Copy link
Contributor Author

gzsombor commented May 6, 2024

Thanks for the explanation, that's a reasonable concern. I guess, src/main/native/zstd_errors.h is also coming from upstream, so you would rather not touch that too, am I right?
I can imagine, creating separate error constants in jni_fast_zstd.c and put the error message somewhere there could work, but I'm afraid that it will be a bit more complicated. Would you merge a solution like that, or any idea, how to solve this problem?

@luben
Copy link
Owner

luben commented May 7, 2024

Right, native/zstd_errors.h also comes from upstream. I also don't think that maintaining our own message types in jni_fast_zstd is good idea.

I think for that case, may be better approach is to check for negative offsets on the Java side, before calling the JNI code and raising an exception from there.

@gzsombor gzsombor closed this May 8, 2024
@gzsombor gzsombor deleted the errormsgs branch May 8, 2024 06:01
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.

2 participants