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

[1.5.0] Promote ZSTD_c_literalCompressionMode to stable params #2581

Merged
merged 1 commit into from May 3, 2021

Conversation

senhuang42
Copy link
Contributor

Promotes ZSTD_c_literalCompressionMode to stable, removes it from the experimental section, and adjusts the rest of the experimentalParams numberings accordingly.

lib/zstd.h Outdated
ZSTD_c_experimentalParam12=1009,
ZSTD_c_experimentalParam13=1010,
ZSTD_c_experimentalParam14=1011
ZSTD_c_experimentalParam5=1003,
Copy link
Contributor

@Cyan4973 Cyan4973 Apr 16, 2021

Choose a reason for hiding this comment

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

Changing the enum nb of a parameter might lead to some downstream issues.

Some binders in other languages, such as Java,
may not access the enum definition, and use the known integer value directly.
In which case, changing the meaning of this integer can lead to downstream surprises.

Of course, this is a very niche scenario, so I don't anticipate a lot of real-life issues,
but we also have nothing to gain from introducing this disruption.

So I would keep the other numbers "as is", and accept holes in the number scheme.

edit : reading about it again,
it seems you kept the integer value,
but changed instead the associated intermediate enum.
This is milder, since the above scenario (using integer value directly) is properly covered.
Yet, I fail to see a benefit from doing this update of intermediate enum. Is there one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was more just a visual decision, so that the whole range of ZSTD_c_experimentalParam[1-n] is represented. But, I can see why it might just be preferable to delete it, since it shouldn't really matter to anyone experimentalParam* wouldn't be consecutive.

@senhuang42 senhuang42 force-pushed the lcm_stable branch 2 times, most recently from f18fa8d to 1e77a33 Compare April 16, 2021 18:42
@ghost
Copy link

ghost commented Apr 17, 2021

Just take this opportunity to make a suggestion.
It's better not to use the same values in ZSTD_cParameter and ZSTD_dParameter.
For example, these two values are both 100:

typedef enum {
    ZSTD_c_compressionLevel=100,
    ...
} ZSTD_cParameter;

typedef enum {
    ZSTD_d_windowLogMax=100, 
    ...
} ZSTD_dParameter;

If an user incorrectly sets the decompression parameter to a compression context, it's very hard to find such bug.

In addition, please create a meta issue for v1.5.0, then the users can know changes and feedback earlier.

@terrelln
Copy link
Contributor

If an user incorrectly sets the decompression parameter to a compression context, it's very hard to find such bug.

We can try to avoid it in the future, but we cannot change the values which are already stable. This will be caught in C++ source code, because an implicit conversion from ZSTD_cParameter to ZSTD_dParameters is disallowed. Additionally, passing -Werror=enum-conversion to clang/gcc will also raise an error for this bug.

@@ -332,6 +332,10 @@ typedef enum {
* The higher the value of selected strategy, the more complex it is,
* resulting in stronger and slower compression.
* Special: value 0 means "use default strategy". */
ZSTD_c_literalCompressionMode=108, /* Controls how the literals are compressed (default is auto).
* The value must be of type ZSTD_literalCompressionMode_e.
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation states that The value must be of type ZSTD_literalCompressionMode_e.

This implies that ZSTD_literalCompressionMode_e must also be part of "stable" public API.

lib/zstd.h Outdated
@@ -332,6 +332,10 @@ typedef enum {
* The higher the value of selected strategy, the more complex it is,
* resulting in stronger and slower compression.
* Special: value 0 means "use default strategy". */
ZSTD_c_literalCompressionMode=108, /* Controls how the literals are compressed (default is auto).
* The value must be of type ZSTD_literalCompressionMode_e.
* See ZSTD_literalCompressionMode_t enum definition for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely a typo:
ZSTD_literalCompressionMode_t => ZSTD_literalCompressionMode_e

@ghost
Copy link

ghost commented Apr 22, 2021

This will be caught in C++ source code, because an implicit conversion from ZSTD_cParameter to ZSTD_dParameters is disallowed. Additionally, passing -Werror=enum-conversion to clang/gcc will also raise an error for this bug.

For some language bindings, this may be a problem, especially for dynamic languages.
On the whole, it's not a big problem, just a suggestion for the future.

@senhuang42 senhuang42 marked this pull request as ready for review April 26, 2021 13:55
@senhuang42 senhuang42 merged commit 4c5cc34 into facebook:dev May 3, 2021
@senhuang42 senhuang42 deleted the lcm_stable branch May 12, 2021 19:22
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.

None yet

4 participants