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

Deprecated bufferless and block level APIs #3534

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Mar 8, 2023

  • Mark all bufferless and block level functions as deprecated
  • Update documentation to suggest not using these functions
  • Add _deprecated() wrappers for functions that we use internally and call those instead
  • Leave the bufferless decompression API as-is for now

I've left the buffer-less decompression API as-is, because I feel that it is less risky to use, since the decompression code is simpler, and is changed less often. Additionally, this is where the additional copies the streaming API does has a bigger impact. I think we should still deprecate it, but maybe we do want a replacement before we do so. Up for debate.

@terrelln terrelln force-pushed the 2023-01-24-deprecate-bufferless-api branch from 250b661 to 9d8962c Compare March 8, 2023 01:43
@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 9, 2023

I'm just wondering if the _deprecated() suffix for internal functions still in use within libzstd is appropriate.
This suffix implies that these symbols will be removed in some future, and therefore should be actively avoided.
But I don't see any action in this direction.

It seems we just want to avoid their invocation directly from user space,
but internally, they will remain in use for the foreseeable future.

In which case, would another suffix be more appropriate ? like _internal() for example ?

@terrelln
Copy link
Contributor Author

In which case, would another suffix be more appropriate ? like _internal() for example ?

I chose deprecated because half of these functions already had an _internal() version. I think deprecated is fairly clear if used consistently. We're calling this function because the normal function is marked deprecated.

Though I'm fine with a different prefix that is neither _internal() nor _deprecated().

@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 11, 2023

I understand your point.
_internal() is already in use in several cases, so you need an alternative.

On my side, I'm concerned that _deprecated() suffix strongly implies that we want to get rid of these functions,
and that doesn't always seem to be the case, in which case it would be a misleading signal.

And now it becomes a case-by-case analysis,
and I presume that's exactly the kind of time-waster you wanted to avoid.

So let me get into this kind of details for you.

  • ZSTD_compressContinue_deprecated() : this one seems used in too many places to be removed. Granted, all it does is forward to ZSTD_compressContinue_internal(), and enrich it with 2 advanced parameters. But given that these parameters are not terribly intelligible, I believe there is still some value in hiding this complexity. We'll probably be employing it for a long time, hence we need another name.
    Since _internal() is already taken, and ZSTD_compressContinue() is about continuing compression of a frame without finishing it, we could use a more explicit suffix, for example ZSTD_compressContinue_unfinished() ? We could also completely change the name, and employ ZSTD_compressChunk() for example.

  • ZSTD_compressEnd_deprecated() : this is the same story, although in this case the function body is actually complex. This symbol is here to stay, for long. We need another name. Good news : ZSTD_compressEnd_internal() is available!

  • ZSTD_compressBegin_usingCDict_deprecated() : this symbol is only used once, in zdict unit, and doesn't do much (just add a structure parameter), so we could definitely imagine removing it in the future. In this case _deprecated() feels appropriate.

  • ZSTD_compressBlock_deprecated() : same story, only used once, in zdict. In this case though, I'm not even sure if ZSTD_compressBlock() is even necessary, it could probably be replaced by the more common ZSTD_compressCCtx(), eliminating the need for an additional symbol. Here also, the _deprecated() suffix feels appropriate.

  • ZSTD_getBlockSize_deprecated() : only used once, as part of ZSTD_compressBlock_deprecated(), which might not be needed anymore, so yeah, _deprecated() is fine here too.

@terrelln
Copy link
Contributor Author

Sounds good, I'll rename the functions

@terrelln
Copy link
Contributor Author

@Cyan4973 How about ZSTD_compressContinue_public() and ZSTD_compressEnd_public()? I like the _public() suffix because it suggests that it is exactly the same as ZSTD_compressContinue() and ZSTD_compressEnd()? Which is the intention.

@terrelln terrelln force-pushed the 2023-01-24-deprecate-bufferless-api branch from 9d8962c to 8317dd8 Compare March 14, 2023 22:55
@Cyan4973
Copy link
Contributor

How about ZSTD_compressContinue_public() and ZSTD_compressEnd_public()? I like the _public() suffix because it suggests that it is exactly the same as ZSTD_compressContinue() and ZSTD_compressEnd()?

That's fine by me.

@terrelln terrelln force-pushed the 2023-01-24-deprecate-bufferless-api branch from 8317dd8 to 71670f9 Compare March 14, 2023 23:18
* Mark all bufferless and block level functions as deprecated
* Update documentation to suggest not using these functions
* Add `_deprecated()` wrappers for functions that we use internally and
  call those instead
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