GeezerVison is showing github.com — Document the preferred style for API functions with three, four or five-way returns · Issue #1121 · python/devguide
Document the preferred style for API functions with three, four or five-way returns #1121DescriptionMost C API functions either succeed or fail. Those functions return NULL/not-NULL or 0/-1. No problem there.
However there are functions that can have three or more return kinds.
For example, found/not-found/error, yield/return/raise, or even less-than/equal/greater-than/unordered/error in the case of comparisons.
For those functions we want to return the kind as an int, setting any PyObject * results through an out pointer.
The devguide should document the preferred way to do this, to avoid endless debate for each new C API function.
See python/cpython#105201 (comment) for my preferred approach for three-way return values.
Four and five way return values other than for comparisons are sufficiently uncommon that they can be dealt with on an individual basis.
For comparisons, a slightly strange enum based on powers-of-two is to be used for efficient selection of the desired result by masking. Reacted by Erlend E. Aasland Activityvstinner commented on Jun 23, 2023 While it's different, here is an example of API using different negative values for result.
The private _Py_DecodeLocaleEx() function returns 0 on success, or a negative number on error:
- -1 on memory allocation failure
- -2 on decoding error
- -3 if the error handler is not supported
markshannon commented on Jun 23, 2023 Since _Py_DecodeLocaleEx() is a private function, it can do whatever it wants 🙂
Having said that, it is possible that an API function would want to distinguish between error types.
I don't think that is worth changing the rules for some hypothetical use case, though.
Are there any other examples? erlend-aasland commented on Jun 23, 2023 AFAICS, the proposed guideline is:
- For functions returning failure or success, the return value is respectively -1 and 0.
- For functions returning failure, lesser result (success), greater result (success), the return values are respectively -1, 0, and 1.
I'm +1 for those two guidelines.
Also, to avoid any potential confusion: when we're talking about returning, I interpret that as the C return statement; I do not interpret that as an "output parameter" (for example a pointer to an int). Example:
// This function _returns_ -1 in case of error, else 0.
int
func(void)
{
/* do stuff */
if (error) {
raise_exception();
return -1;
}
return 0;
}markshannon commented on Jun 23, 2023 For functions that return (the C return statement) two different kinds of values, yes.
But this issue is about functions that (C) return three or more different values, like PyDict_GetItem[Suffix].
I think we are all in agreement that in the case of failure, -1 should be returned.
Opinions differ on how to handle success variants. vstinner commented on Jun 23, 2023
For functions returning failure or success, the return value is respectively -1 and 0.
In PR #106005, adding PyDict_GetItemRef() function, @markshannon asks to return 1 if the key is present and 0 if the key is missing. Is it a bad practice? Is it better to return 0 in both cases (that was my first implementation)? markshannon commented on Jun 23, 2023
For functions returning failure or success, the return value is respectively -1 and 0.
In PR #106005, adding PyDict_GetItemRef() function, @markshannon asks to return 1 if the key is present and 0 if the key is missing. Is it a bad practice? Is it better to return 0 in both cases (that was my first implementation)?
Try reading the next line 😉
For functions returning failure, lesser result (success), greater result (success), the return values are respectively -1, 0, and 1.
erlend-aasland commented on Jun 23, 2023 erlend-aasland commented on Jun 23, 2023
For functions that return (the C return statement) two different kinds of values, yes.
[...]
I think we are all in agreement that in the case of failure, -1 should be returned.
Sure; but I prefer to document it so there's even less chance of friction :) markshannon commented on Jun 23, 2023 One other issue is whether the output parameter should be touched if there is no meaningful value.
Take PyDict_GetItemSuffixToBeDecided(PyDictObject *op, PyObject *key, PyObject **pvalue).
Should we touch *pvalue if returning -1 or 0.
In other words, in
PyObject *value;
int err = PyDict_GetItemSuffixToBeDecided(op, key, &value);Should value be undefined if err <= 0, or should it be set to NULL?
It is more efficient to not touch it, since the return value specifies that it has no meaning.
But, do we trust all C extension authors not to read the value, which may point to a real object if they are unlucky? Setting *pvalue = NULL is safer. Reacted by Erlend E. Aasland erlend-aasland commented on Jun 23, 2023
One other issue is whether the output parameter should be touched if there is no meaningful value.
Yeah... let's establish guidelines for the return values first; then we can follow up with guidelines for output parameters. vstinner commented on Jun 23, 2023
Should value be undefined if err <= 0, or should it be set to NULL?
In my PyDict_GetItemRef() PR, I chose to always initialized *pvalue, especially in the error case (return -1). IMO there is a minor performance overhead, but it closes a whole class of issue: undefined behavior if the function returns -1. Some C compilers try to evaluate all code paths and say that a &value argument "can be undefined" when the code is correct. A common workaround makes me sad: initialize the value in the calling site: PyObject *value = NULL; :-(
For the "return 0" case (missing key), I also prefer to set *pvalue to NULL for the same reason, but also because people upgrading their code from PyDict_GetItem() and PyDict_GetItemWithError() may want to check if value is NULL or not, rather than checking the return value:
PyObject *value; // OMG! undefined value
if (PyDict_GetItemRef(dict, key, &value) < 0) ... error ...
else if (value == NULL) ... missing key ...
else ... key is present ...
Here the function result is not used for the second code path: ... missing key ....
I'm not comfortable to document that *pvalue is intialized for the error case (return -1). But I wrote an unit test for that. encukou commented on Jun 26, 2023
I think we are all in agreement that in the case of failure, -1 should be returned.
IMO, a negative value should be returned. Functions should return -1 unless they have a reason to use something else (like distinguish between error types), but callers should check with < 0.
Also, IMO we should say that a negative value should be returned iff an exception is raised. I.e., make it very clear what counts as “failure”. Reacted by Erlend E. Aasland erlend-aasland commented on Jun 26, 2023
IMO, a negative value should be returned. Functions should return -1 unless they have a reason to use something else (like distinguish between error types), but callers should check with < 0.
Also, IMO we should say that a negative value should be returned iff an exception is raised. I.e., make it very clear what counts as “failure”.
Adding something like this would indeed make the guidelines clearer. I'll create PR with an amendment shortly. willingc commented on Oct 13, 2023 erlend-aasland commented on Oct 13, 2023 We will leave this to the C API Workgroup. If needed, they can create a new issue or reopen this.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|