gh-132983: Split _zstd_set_c_parameters
#133921
Conversation
3271d2c to
d44dd9b
Compare
| _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.
There is no need to use all these consts. 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.
key and value are borrowed references. Incref them before calling PyLong_AsInt().
| return -1; | ||
| } | ||
| const int key_v = PyLong_AsInt(key); | ||
| if (key_v == -1 && PyErr_Occurred()) { |
There was a problem hiding this comment.
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.
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 what set_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.
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."); |
| # 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.
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) |
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, |
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 when level 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 the level parameter. I can see reasonable arguments to keep the status quo, however, so I've not done this yet.
A
cc @Rogdham