-
Notifications
You must be signed in to change notification settings - Fork 1
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return value conventions #13
Comments
@encukou Can you edit this a bit? It looks like some kind of copy/paste error. Also, I would avoid the loaded term "success", at least for int-returning functions -- for cases like We might also want to call out that there are families of existing APIs that return 0 for success and 1 for error (following shell conventions), and others that return 0 for error and 1 for success. So we will have to continue documenting the return value in each case. (Less so for functions that return |
Edited, thanks. IMO, we should always document return values -- at least have a summary with a link. Not everyone reads the section that applies to all of the API. |
I vote for always setting the result to something, and document this. If the result is an object, in the error case the result should be |
|
That's better API. |
See also capi-workgroup/problems#1 "Ambiguous return values". |
See also python/devguide#1129 "Improve C API guidelines for return values". |
See also python/devguide#1125 "C API: Add guidelines for C APIs with output params". |
Added a bit more related discussion on #40 (comment) but I think an additional key question is: how do we return both a value and some additional information about where that value came from? An existing example would be Should we stick to this convention, e.g. Or do we want to switch to only returning exception status and all values go through output parameters, e.g. Or combine the information about the value with the error status and use that as the result, e.g. (Note that I'm not proposing changing this particular API, just illustrating how it may have been done under different designs.) |
IMO the API should return directly the value, unless there is an ambiguous case and it's common to have to distinguish them.
There is also a desire to avoid the need to call PyErr_Occurred(). Another concern is designing API to not make them error-prone: encourage the author to handle all cases, especially to handle the error case (if an exception is set). There is another category: "optional parameters". Sometimes, you might want to provide additional output arguments to the caller, but the caller may not use them. In PR python/cpython#112028 I propose In the PyUnicode_AsUTF8AndSize() case, IMO size can be seen as optional (and it can be set to NULL): Python has code which calls In the proposed PyImport_ImportOrAddModule() API, maybe the information "was the module created?" can be seen as optional, especially because currently the API does not provide the information: it's a new feature. So there are different flavors of APIs:
The case (A) covers a large part of the C API. Except that some functions require calling PyErr_Occurred() to write correct code (to properly handle errors). |
Yeah, I largely agree with Victor's comments. Particularly:
I'd frame this as "when distinguishing a valid/expected result from an error". We don't have a useful way to return "not found" as a
I'd argue that it can be seen as optional because the post-condition of the function is that a module exists and was returned. This is met in both cases, whether the module existed or is new, and so whether it was just created is optional. Contrast this with |
I'd prefer using (A) and (C) only -- i.e. switch to output parameters (and returning int status) whenever the return value isn't enough for all information the function provides.
I'd argue that for consistency, we should simply allow all output parameters to be |
It's kind of surprising to call |
It's a shortcut for
I'd love to see the benchmark behind that claim. Especially with PGO. |
Sure, it should be measured. @serhiy-storchaka was worried about that in my python/cpython#112028 PR. |
It is exactly the interface of |
(From the proposal in the first post:)
Hash functions return -1 on error, but all other values, including negative ones, are valid. |
Right, it's part of |
Added to the draft in #53, with a but of wording that might be ambiguous:
That doesn't explicitly say if we want to allow functions like There's a current decision issue on the topic: capi-workgroup/decisions#48 |
Supporting NULL as an output argument can be too costly for fast functions, it can hit performance of the common case. Specialized function for checking for existence also can be more efficient. It may be impractical to support this as a strong general rule. |
On the other hand, specialized functions for cases where "I don't care about this value" is perfectly clear, leads to API bloat and makes it harder to figure out how to write the correct code (time-of-check-time-of-use-etc.). We also end up maintaining more code on the implementation side - a null check before writing an output is one (short) line of code, whereas an entirely separate exported function is going to touch 2-3 files minimum. I'm also skeptical that "indirected write" vs "null check and indirected write" is going to come out clearly in favour of either path. I'm sure either case can be proven with microbenchmarks, but I really can't see it mattering that much in the overall scheme of things. Especially since we should be doing null checks on output argument pointers anyway. Macros and inline functions will optimise away literal null arguments just fine. I'd prefer to continue relying on macros and inline functions for fast functions, and optimise for code size and maintainability for exported ones. |
New API functions can return:
-1
is returned if and only if an exception was raised, and non-negative values signal an absence of exception.Negative values other than(edit: they are, for hashes -- see below)-1
are never returned.NULL
is returned if and only if an exception was raised.PyStatus
for pre-initialization functions.)The return values need to be documented.
Some existing functions don't follow this convention.
If
NULL
or a negative number are valid outputs, the function needs to reserve the return value for signaling errors, and take an extra*result
argument that it fills.XXX: Is
*result
set on failure?"Errors" that you expect to be common, where you want to avoid the overhead of creating an exception object (like the
AttributeError
from a getattr), aren't treated as failures. In these cases, return0
for the expected "error", and 1 for complete success.XXX: is
*result
set in this case?The text was updated successfully, but these errors were encountered: