Fix returning null from functions with output parameters in Python #2907
Conversation
|
Won't this result in |
Null pointers returned from functions also using output parameters or C strings typemaps were lost because the generated code didn't distinguish between Py_None corresponding to them and Py_None indicating the return value of a "void" function. This doesn't seem to be easy to solve properly because the function return type and output parameters typemaps are independent and there is no way to customize the behaviour of the latter depending on the former. So we do it using a hack involving a global variable which is always 0 and is passed to SWIG_Python_AppendOutput() from non-void functions, but is shadowed by a local variable with the same name set to 1 in the void ones. This allows us to only discard Py_None if it corresponds to a void function return value. This is obviously ugly, but requires only minimal changes, doesn't impose much extra run-time overhead and is MT-safe (unlike any solution involving changing global variables). Also add a unit test checking for this for Python and for JS, where the same function already worked correctly (unlike in some other languages, including at least Ruby and PHP). Closes swig#2905.
vadz
force-pushed
the
py-nullptr-with-outparam
branch
from
355b8a2
to
6a49871
Compare
It will (as well as And this was the only way to make this work that I found. I don't know if I should feel proud or horrified, but I think this solution is nice, in spite of being so ugly, because it's pretty minimal, both at SWIG and at the generated code level. But I'll obviously need to wait to see what @wsfulton thinks of it. I hope it can be accepted because it solves a real problem, but if not, it won't be the first patch I'll just maintain in my own fork. |
|
Warnings in generated code do seem of somewhat limited use, though SWIG generated files can contain significant amounts of non-generated code from the user interface file and they can be useful there. Also it's often much simpler to enable warnings across a whole project. For whatever reasons, enough people expect SWIG-generated code not to trigger warnings that knowingly introducing such warnings is just setting up for future bug reports we then have to deal with. Better to address this now while it's fresh in our minds, and we already have |
|
Sorry, I don't know how to avoid |
|
Oh true Pretty much any code will generate warnings if you turn on obscure enough warning options, but it seems reasonable to aim for being clean with |
|
I couldn't find a way to use diff --git a/Lib/python/pytypemaps.swg b/Lib/python/pytypemaps.swg
index e1f649885..3b14fb232 100644
--- a/Lib/python/pytypemaps.swg
+++ b/Lib/python/pytypemaps.swg
@@ -81,7 +81,8 @@
than the return value of a void function. By intentionally shadowing the
global swig_is_void which is always 0 by this local variable set to 1 here,
we can tell it to do this only for void functions. */
-%typemap(out,noblock=1) void (int swig_is_void = 1) {$result = VOID_Object;}
+%typemap(out,noblock=1) void (int swig_is_void = 1) {(void)swig_is_void;
+ $result = VOID_Object;}
/* Consttab, needed for callbacks, it should be removed later */
which fixes the warnings given by To avoid rerunning the CI uselessly, I'll wait for William's review before doing it. |
|
@wsfulton This is yet another (relatively) simple bug fix correcting a real problem in SWIG and yet there has been no reaction since almost a month. I'd really appreciate if it could be applied or if I could get at least some feedback about it. TIA! |
|
Hi @vadz, sure, I can look at this one today for you. |
|
Clever hack, but I can put in a proper fix. I've added 790f7d6 to your py-nullptr-with-outparam branch rebased on master with a quick implementation of it. It's quite similar to the |
|
Thanks, this is indeed a much better solution (but I didn't think about it as I wanted to do it entirely at typemaps level, for some reason)! If you'd like, I can try to clean it up and generalize to the other languages now — but if you'd prefer to do it yourself, I wouldn't mind at all, of course, just please let me know. Thanks again! |
|
Thanks, but I can clean it up. I spotted some deprecated stuff and a few quirks in this area while looking at it that I'd like to deal with. |
The $isvoid special variable expands to 1 if the wrapped function has a void return, otherwise expands to 0. In the implementation, use a consistent variable name, returntype, for a node's "type" attribute. Issue #2907
|
See commits f3d4698...d940190 |
Null pointers returned from functions also using output parameters or C strings typemaps were lost because the generated code didn't distinguish between Py_None corresponding to them and Py_None indicating the return value of a "void" function.
This doesn't seem to be easy to solve properly because the function return type and output parameters typemaps are independent and there is no way to customize the behaviour of the latter depending on the former.
So we do it using a hack involving a global variable which is always 0 and is passed to SWIG_Python_AppendOutput() from non-void functions, but is shadowed by a local variable with the same name set to 1 in the void ones. This allows us to only discard Py_None if it corresponds to a void function return value.
This is obviously ugly, but requires only minimal changes, doesn't impose much extra run-time overhead and is MT-safe (unlike any solution involving global variables).
Also add a unit test checking for this for Python and for JS, where the same function already worked correctly (unlike in some other languages, including at least Ruby and PHP).
Closes #2905.