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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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. |
# Conflicts:#Modules/_zstd/compressor.c
| @@ -196,10 +196,17 @@ def test_simple_compress_bad_args(self): | |||
| self.assertRaises(TypeError, ZstdCompressor, zstd_dict=b"abcd1234") | |||
| self.assertRaises(TypeError, ZstdCompressor, zstd_dict={1: 2, 3: 4}) | |||
| # valid compression level range is [-(1<<17), 22] | |||
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.
What are the error messages for out of range values?
| ZstdDecompressor(options={2**31: 100}) | ||
| with self.assertRaises(ValueError): | ||
| ZstdDecompressor(options={2**1000: 100}) | ||
| with self.assertRaises(ValueError): | ||
| ZstdDecompressor(options={-(2**31)-1: 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.
You perhaps meant to test for2**31-1 and-2**31.
| options[DecompressionParameter.window_log_max] = 2**31 | ||
| with self.assertRaises(ValueError): | ||
| decompress(b'', options=options) | ||
| options[DecompressionParameter.window_log_max] = -(2**32)-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.
| @@ -49,98 +49,108 @@ typedef struct { | |||
| #include "clinic/compressor.c.h" | |||
| static int | |||
| _zstd_set_c_parameters(ZstdCompressor *self, PyObject *level_or_options, | |||
| const char *arg_name, const char* arg_type) | |||
| _zstd_set_c_level(ZstdCompressor *self, const int level) | |||
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.
const is redundant here.
| return -1; | ||
| } | ||
| Py_INCREF(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.
It is never decrefed.
| "Key of options dict should be either a " | ||
| "CompressionParameter attribute or an int."); | ||
| return -1; | ||
| "dictionary key must be less than 2**31"); |
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.
This error message does not look quite useful (first, you can get OverflowError for negative values whichare less than2**31). What error do you get if the key value is in the range of C int, but is not valid? Try to imitate it. If it is too difficult, leave an OverflowError, it can be improved in future.
BTW, the old error message was about the type of the key, not its value. TypeError is more suitable for it. Do you have tests for non-integer key or value?
| PyErr_SetString(PyExc_ValueError, | ||
| "Value of options dict should be an int."); | ||
| return -1; | ||
| "dictionary value must be less than 2**31"); |
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 for error message for key.
| if (level != PY_SSIZE_T_MIN && options != Py_None) { | ||
| PyErr_SetString(PyExc_RuntimeError, "Only one of level or options should be used."); | ||
| if (level != Py_None && options != Py_None) { | ||
| PyErr_SetString(PyExc_RuntimeError, |
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.
TypeError is more suitable for calling with conflicting arguments.
| PyErr_Format(PyExc_ValueError, | ||
| "%zd not in valid range %d <= compression level <= %d.", | ||
| level, ZSTD_minCLevel(), ZSTD_maxCLevel()); |
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.
This is a good example for enhanced ValueError message. But%zd is not valid format forPyObject *. Taking into account that string representation oflevel can be arbitrary long, it is better to not include it in the error message.
| Py_INCREF(key); | ||
| int key_v = PyLong_AsInt(key); | ||
| if (key_v == -1 && PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "Key of options dict should be either a " | ||
| "DecompressionParameter attribute or an int."); | ||
| if (PyErr_ExceptionMatches(PyExc_OverflowError)) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "dictionary key must be less than 2**31"); | ||
| } | ||
| return -1; | ||
| } | ||
| Py_INCREF(value); | ||
| int value_v = PyLong_AsInt(value); | ||
| if (value_v == -1 && PyErr_Occurred()) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "Value of options dict should be an int."); | ||
| if (PyErr_ExceptionMatches(PyExc_OverflowError)) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "dictionary value must be less than 2**31"); | ||
| } |
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 issues as for compressor.
When you're done making the requested changes, leave the comment: |
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