[bug-fix] Make simple single-pass functions ignore advanced parameters #2498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
terrelln
commented
Feb 13, 2021
| params.cParams = ( pledgedSrcSize < ZSTD_USE_CDICT_PARAMS_SRCSIZE_CUTOFF | ||
| || pledgedSrcSize < cdict->dictContentSize * ZSTD_USE_CDICT_PARAMS_DICTSIZE_MULTIPLIER | ||
| || pledgedSrcSize == ZSTD_CONTENTSIZE_UNKNOWN | ||
| || cdict->compressionLevel == 0 ) | ||
| && (params.attachDictPref != ZSTD_dictForceLoad) ? |
There was a problem hiding this comment.
@felixhandte note this small behavior change with regards to attaching dictionaries. This function is used by ZSTD_compress_usingCDict*(), which doesn't use requestedParams anymore.
Cyan4973
reviewed
Feb 13, 2021
lib/compress/zstd_compress.c
Outdated
| cctxParams->cParams = params.cParams; | ||
| cctxParams->fParams = params.fParams; | ||
| cctxParams->compressionLevel = ZSTD_NO_CLEVEL; /* should not matter, as all cParams are presumed properly defined */ | ||
| ZSTD_CCtxParams_init_internal(cctxParams, ¶ms, ZSTD_NO_CLEVEL); | ||
| return 0; | ||
| } | ||
|
|
||
| /* ZSTD_assignParamsToCCtxParams() : |
There was a problem hiding this comment.
minor: update name in code comment
terrelln
force-pushed
the
old-api-fix
branch
from
bfd85fb to
7736549
Compare
Cyan4973
approved these changes
Feb 13, 2021
The simple compression functions are intended to ignore the advanced parameters, but they were accidentally using them. All the `ZSTD_parameters` were set correctly, but any extra parameters were used as-is. E.g. `ZSTD_c_format`. This PR makes all the simple single-pass functions listed below ignore the advanced parameters, as intended. * `ZSTD_compressCCtx()` * `ZSTD_compress_usingDict()` * `ZSTD_compress_usingCDict()` * `ZSTD_compress_advanced()` * `ZSTD_compress_usingCDict_advanced()` It also adds a test case that ensures that each of these functions ignore the advanced parameters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The simple compression functions are intended to ignore the advanced
parameters, but they were accidentally using them. All the
ZSTD_parameterswere set correctly, but any extra parameters wereused as-is. E.g.
ZSTD_c_format.This PR makes all the simple single-pass functions listed below ignore
the advanced parameters, as intended.
ZSTD_compressCCtx()ZSTD_compress_usingDict()ZSTD_compress_usingCDict()ZSTD_compress_advanced()ZSTD_compress_usingCDict_advanced()It also adds a test case that ensures that each of these functions
ignore the advanced parameters.