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

[lib] Set appliedParams.compressionLevel correctly #2497

Merged
merged 1 commit into from
Feb 13, 2021

Conversation

terrelln
Copy link
Contributor

Forward the correct compressionLevel to the appliedParams in all cases.
It was already correct for the advanced API, so only the old single-pass
functions needed to be fixed.

This compression level is unused by the library, but is set so that the
tracing framework can consume it.

Forward the correct compressionLevel to the appliedParams in all cases.
It was already correct for the advanced API, so only the old single-pass
functions needed to be fixed.

This compression level is unused by the library, but is set so that the
tracing framework can consume it.
@@ -3614,7 +3621,7 @@ size_t ZSTD_compress_usingDict(ZSTD_CCtx* cctx,
int compressionLevel)
{
ZSTD_parameters const params = ZSTD_getParams_internal(compressionLevel, srcSize, dict ? dictSize : 0, ZSTD_cpm_noAttachDict);
ZSTD_CCtx_params cctxParams = ZSTD_assignParamsToCCtxParams(&cctx->requestedParams, &params);
ZSTD_CCtx_params cctxParams = ZSTD_assignParamsToCCtxParams(&cctx->requestedParams, &params, (compressionLevel == 0) ? ZSTD_CLEVEL_DEFAULT: compressionLevel);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor : 0 or ZSTD_NO_CLEVEL ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking 0 because the semantic isn't that no compression level is set, but its a default. I am using ZSTD_NO_CLEVEL only when no compression level is set.

We could add a second macro for this meaning of 0, but I'm not quite sure what I'd name it.

@terrelln terrelln merged commit f39178b into facebook:dev Feb 13, 2021
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.

3 participants