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

Documentation for ZSTD_CCtx_reset() is misleading about parameters #1094

Closed
indygreg opened this issue Apr 1, 2018 · 3 comments
Closed

Documentation for ZSTD_CCtx_reset() is misleading about parameters #1094

indygreg opened this issue Apr 1, 2018 · 3 comments
Assignees
Labels

Comments

@indygreg
Copy link
Contributor

indygreg commented Apr 1, 2018

From zstd.h:

/*! ZSTD_CCtx_reset() :
 *  Return a CCtx to clean state.
 *  Useful after an error, or to interrupt an ongoing compression job and start a new one.
 *  Any internal data not yet flushed is cancelled.
 *  Dictionary (if any) is dropped.
 *  All parameters are back to default values.
 *  It's possible to modify compression parameters after a reset.
 */
ZSTDLIB_API void ZSTD_CCtx_reset(ZSTD_CCtx* cctx);

If we look at zstd_compress.c:

static void ZSTD_startNewCompression(ZSTD_CCtx* cctx)
{
    cctx->streamStage = zcss_init;
    cctx->pledgedSrcSizePlusOne = 0;
}

/*! ZSTD_CCtx_reset() :
 *  Also dumps dictionary */
void ZSTD_CCtx_reset(ZSTD_CCtx* cctx)
{
    ZSTD_startNewCompression(cctx);
    cctx->cdict = NULL;
}

I interpreted All parameters are back to default values to mean ZSTD_CCtx_params is reset to defaults, which would mean callers would need to repopulate those parameters after calling ZSTD_CCtx_reset(). However, we can clearly see from the code that only the internal stream stage, pledged source size, and dictionary are reset. The ZSTD_CCtx_params are untouched.

This confusion almost caused me to add a ZSTD_CCtx_setParametersUsingCCtxParams() after every ZSTD_CCtx_reset() call.

I think the documentation would be better if it clarified which parameters were and were not impacted.

@Cyan4973 Cyan4973 added the bug label Apr 1, 2018
@Cyan4973
Copy link
Contributor

Cyan4973 commented Apr 1, 2018

Thanks for very detailed report @indygreg .
This function clearly does not live up to its definition.
I believe the definition is correct, it's the implementation which is not.
To be fixed.

@terrelln
Copy link
Contributor

I think we should separate the reset function from the clear parameters function. A lot of uses I see want to use the same parameters, but just start a new compression.

What if we kept ZSTD_CCtx_reset() the same, and have a new function called ZSTD_CCtx_resetParameters()?

@Cyan4973
Copy link
Contributor

Sounds good to me

@terrelln terrelln self-assigned this Apr 12, 2018
terrelln added a commit to terrelln/zstd that referenced this issue Apr 12, 2018
* Fix docs for `ZSTD_CCtx_reset()`.
* Add `ZSTD_CCtx_resetParameters()`.

Fixes facebook#1094.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants