Skip to content

[1.5.0] Deprecate some functions #2582

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 6, 2021
Merged

[1.5.0] Deprecate some functions #2582

merged 2 commits into from
May 6, 2021

Conversation

senhuang42
Copy link
Contributor

@senhuang42 senhuang42 commented Apr 16, 2021 •
edited
Loading

We mark some functions as deprecated for the upcoming 1.5.0 release:

Functions that take ZSTD_parameters as an argument:
ZSTD_compress_advanced()
ZSTD_compress_usingCDict_advanced()
ZSTD_initCStream_advanced()
ZSTD_initCStream_usingCDict_advanced()
ZSTD_compressBegin_advanced()
ZSTD_compressBegin_usingCDict_advanced()

Functions that are redundant:
ZSTD_initCStream_srcSize()
ZSTD_initCStream_usingDict()
ZSTD_initCStream_usingCDict()
ZSTD_resetCStream()
ZSTD_DCtx_setFormat()

In addition, we modify some build scripts (Make, CMake, VS) for fuzzer, fullbench, and zstreamtest to ignore deprecated declarations since we still explicitly test some of these functions.

In situations where it is appropriate, we also convert the deprecated API into the current API.

Some remaining zbuff related build targets/paths that were missed in #2583 get removed too.

@senhuang42 senhuang42 force-pushed the deprecate_some_functions branch 7 times, most recently from ff6b140 to 6b6c60a Compare April 26, 2021 14:25
@senhuang42
Copy link
Contributor Author

I've added -Wno-deprecated-declarations to the linux kernel test. I notice that zstd_compress_module.c doesn't really use any of the new compress2-type advanced API, and actually the wrapper functions translate pretty directly to this API that we're deprecating with their usage of ZSTD_parameters.

So I'm not entirely sure whether it would be better to update the internals of these wrapper functions or just let them keep using this API we're deprecating? cc @terrelln

@senhuang42 senhuang42 marked this pull request as ready for review April 26, 2021 15:05
@senhuang42 senhuang42 force-pushed the deprecate_some_functions branch from 6b6c60a to 3e2b6d3 Compare April 26, 2021 16:53
@Cyan4973
Copy link
Contributor

Question: Should we also deprecate ZSTD_initCStream()?

Nope, no need.

@Cyan4973
Copy link
Contributor

We update docs on setFormat() to mention that it is redundant, but don't generate a compiler warning for this one, since it is a stable API entrypoint that can modify an advanced API param.

As far as I know, ZSTD_DCtx_setFormat() is not part of "stable".

@senhuang42 senhuang42 force-pushed the deprecate_some_functions branch 4 times, most recently from 5ce68d6 to e1ea4b7 Compare April 27, 2021 00:00
terrelln
terrelln reviewed May 4, 2021
View reviewed changes
lib/zstd.h Outdated
Comment on lines 38 to 59
/* Deprecation warnings :
* Should these warnings be a problem, it is generally possible to disable them,
* typically with -Wno-deprecated-declarations for gcc or _CRT_SECURE_NO_WARNINGS in Visual.
* Otherwise, it's also possible to define ZSTD_DISABLE_DEPRECATE_WARNINGS.
*/
#ifdef ZSTD_DISABLE_DEPRECATE_WARNINGS
# define ZSTD_DEPRECATED(message) ZSTDLIB_API /* disable deprecation warnings */
#else
# if defined (__cplusplus) && (__cplusplus >= 201402) /* C++14 or greater */
# define ZSTD_DEPRECATED(message) [[deprecated(message)]] ZSTDLIB_API
# elif (defined(GNUC) && (GNUC > 4 || (GNUC == 4 && GNUC_MINOR >= 5))) || defined(__clang__)
# define ZSTD_DEPRECATED(message) ZSTDLIB_API __attribute__((deprecated(message)))
# elif defined(__GNUC__) && (__GNUC__ >= 3)
# define ZSTD_DEPRECATED(message) ZSTDLIB_API __attribute__((deprecated))
# elif defined(_MSC_VER)
# define ZSTD_DEPRECATED(message) ZSTDLIB_API __declspec(deprecated(message))
# else
# pragma message("WARNING: You need to implement ZSTD_DEPRECATED for this compiler")
# define ZSTD_DEPRECATED(message) ZSTDLIB_API
# endif
#endif /* ZSTD_DISABLE_DEPRECATE_WARNINGS */

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we move this to the top of the ZSTD_STATIC_LINKING_ONLY section?

I think that helps convey the message that we only deprecate functions in the unstable portion of our API. People who only use the stable section don't need to think about deprecation.

@senhuang42 senhuang42 force-pushed the deprecate_some_functions branch from e1ea4b7 to 280b74b Compare May 6, 2021 19:18
@senhuang42 senhuang42 merged commit 698f261 into facebook:dev May 6, 2021
@senhuang42 senhuang42 mentioned this pull request May 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrelln terrelln terrelln left review comments

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@senhuang42 @Cyan4973 @terrelln @facebook-github-bot