Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-132983: Split_zstd_set_c_parameters#133921
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
| _zstd_state* const mod_state = PyType_GetModuleState(Py_TYPE(self)); | ||
| if (mod_state == NULL) { | ||
| /* Set integer compression level */ | ||
| const int min_level = ZSTD_minCLevel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is no need to use all theseconsts. They just distract.
| "Value of option dict should be an int."); | ||
| return -1; | ||
| } | ||
| const int key_v = PyLong_AsInt(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
key andvalue are borrowed references. Incref them before callingPyLong_AsInt().
| return -1; | ||
| } | ||
| const int key_v = PyLong_AsInt(key); | ||
| if (key_v == -1 && PyErr_Occurred()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do not override arbitrary exceptions. Check the type of exception before replacing it.
| } | ||
| const int key_v = PyLong_AsInt(key); | ||
| if (key_v == -1 && PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_ValueError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If it was a TypeError, it is preferable to keep a TypeError. If it was an OverflowError, it can be replaced with ValueError, but TypeError may be appropriate too in this context. Or whatset_parameter_error() sets for invalid key values.
| const int key_v = PyLong_AsInt(key); | ||
| if (key_v == -1 && PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "key should be a CompressionParameter attribute."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No period at the end of the error message which is not a full sentence.
| int value_v = PyLong_AsInt(value); | ||
| if (value_v == -1 && PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "options dict value should be an int."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above.
| # valid compression level range is [-(1<<17), 22] | ||
| with self.assertRaises(ValueError): | ||
| ZstdCompressor(2**31) | ||
| with self.assertRaises(ValueError): | ||
| ZstdCompressor(level=-(2**31)) | ||
| with self.assertRaises(ValueError): | ||
| ZstdCompressor(options={2**31: 100}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Test also for values out any C integer range:2**1000 and-2**1000.
| with self.assertRaises(ValueError): | ||
| compress(b'', level_max+1) | ||
| with self.assertRaises(ValueError): | ||
| compress(b'', level_min-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above.
serhiy-storchaka commentedMay 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No, there is not. You cannot distinguish the passed value from the default value. It is better to not using Argument Clinic for conversion of this parameter. BTW, |
If we can't use AC converters due to the default value not being expressible in C, I'm not sure splitting the function makes sense. |
Setting "level" and setting "options" do not share code. Why do they even use the same function? |
They were originally one argument in the pyzstd API, so I suppose splitting it is still useful since they are separate arguments. But I do think we should just take |
@AA-Turner similarly, would you like to pick this up again as well? I'm happy to take this over if you don't have time. Want to get the last of the zstd items done before b2. |
Uh oh!
There was an error while loading.Please reload this page.
The current
_zstd_set_c_parametersshares little between the two possible (int or dict) paths. By splitting the function, we can now use thePy_ssize_tAC converter.One slight change worth noting is that the constructor won't raise an error whenlevel is$-2^{63}$ but will just silently use the default. cc@erlend-aasland if there's a better method/sentinel we can use in the clinic to detect if the value has been set or not.
I also think it might be worth considering removing the level XOR options check, as we could adopt the rule that a compression level set in an options dict overrides thelevel parameter. I can see reasonable arguments to keep the status quo, however, so I've not done this yet.
A
cc@Rogdham