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

Deprecate advanced streaming functions #3408

Merged
merged 14 commits into from
Jan 13, 2023
Merged

Conversation

embg
Copy link
Contributor

@embg embg commented Jan 4, 2023

These functions were scheduled to be deprecated in v1.5.x. Fixes #3405

@Cyan4973: should we deprecate ZSTD_initDStream as well?

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 5, 2023

@Cyan4973: should we deprecate ZSTD_initDStream as well?

ZSTD_initDStream() is part of the stable API,
I believe it's good to stay.

lib/decompress/zstd_decompress.c Outdated Show resolved Hide resolved
tests/zstreamtest.c Outdated Show resolved Hide resolved
@embg embg requested a review from terrelln January 5, 2023 20:27
@embg embg marked this pull request as ready for review January 5, 2023 21:53
@embg embg removed the request for review from terrelln January 9, 2023 18:17
@embg embg marked this pull request as draft January 9, 2023 18:17
@embg
Copy link
Contributor Author

embg commented Jan 9, 2023

Converting back to draft status to fix lz4-threadpool-libs test

@embg
Copy link
Contributor Author

embg commented Jan 11, 2023

Ready for review. I decided to #define ZSTD_DISABLE_DEPRECATE_WARNINGS in zbuff.h to suppress the warning for ZBUFF_decompressInitDictionary().

I originally tried this:

 size_t ZBUFF_decompressInitDictionary(ZBUFF_DCtx* zbd, const void* dict, size_t dictSize)
 {
+    {   const size_t resetRet = ZSTD_DCtx_reset(zbd, ZSTD_reset_session_only);
+        if (ZSTD_isError(resetRet)) return resetRet;   }
+    {   const size_t loadRet = ZSTD_DCtx_loadDictionary(zbd, dict, dictSize);
+        if (ZSTD_isError(loadRet)) return loadRet;   }
+    return ZSTD_startingInputLength(zbd->format);
-    return ZSTD_initDStream_usingDict(zbd, dict, dictSize);
 }

The problem is that ZBUFF_DCtx is an opaque struct, so we can't access zbd->format. Rather than including non-public zstd headers to get the definition of the struct, I felt it's cleaner to just suppress the deprecation warning. I'm open to other solutions, though.

@embg embg marked this pull request as ready for review January 11, 2023 21:26
@Cyan4973
Copy link
Contributor

Ready for review. I decided to #define ZSTD_DISABLE_DEPRECATE_WARNINGS in zbuff.h to suppress the warning for ZBUFF_decompressInitDictionary().

zbuff itself is deprecated.
Do you need to #define ZSTD_DISABLE_DEPRECATE_WARNINGS in zbuff.h , or could it be defined in zbuff.c instead ?
This would avoid propagation of this constant.

@embg
Copy link
Contributor Author

embg commented Jan 12, 2023

Do you need to #define ZSTD_DISABLE_DEPRECATE_WARNINGS in zbuff.h , or could it be defined in zbuff.c instead ? This would avoid propagation of this constant.

@Cyan4973 Good point, moved it to zbuff_decompress.c.

@embg
Copy link
Contributor Author

embg commented Jan 12, 2023

Rebased. There seems to be an unrelated CI issue with the mingw tests, will hold off landing this until I figure that out.

@Cyan4973
Copy link
Contributor

Rebased. There seems to be an unrelated CI issue with the mingw tests, will hold off landing this until I figure that out.

The last PR to be merged was #3410. Could it be related ?

@embg
Copy link
Contributor Author

embg commented Jan 12, 2023

@cyan I confirmed that the CI failure is not related to the changes from this PR. (See #3420 for details). Since all other tests are passing, can I land?

@embg embg merged commit 5d8cfa6 into facebook:dev Jan 13, 2023
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark v1.5.x deprecated functions as deprecated
4 participants