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?
Uh oh!
There was an error while loading.Please reload this page.
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.
| 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| # 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