Don't always use "*args" for all Python wrapper functions. #201

Merged
merged 5 commits into from Oct 28, 2014

2 participants

@vadz
SWIG member

Due to what seems like a bug introduced during Python 3 support merge, all the
generated Python functions used the general "*args" signature instead of using
the named parameters when possible.

This happened due to is_primitive_defaultargs() always returning false for the
functions without any default arguments as "value" passed to convertValue()
was NULL in this case and convertValue() always returns false for NULL.

Fix this by checking for value being non-NULL before calling convertValue().

And also check for INOUT parameters for which we do need to use "*args" as we
don't have any name for them. Finally, rename the function to reflect that it
does more than just check for the default arguments now.

@vadz vadz Remove long line wrapping from Python parameter list generation code.
This doesn't play well with PEP8 checks which imposes very strict continuation
line indentation rules which need to be _visually_ aligned, i.e. the subsequent
lines must be indented by the position of the opening bracket in the function
declaration line, but the code generating the parameter lists doesn't have
this information and so it's impossible to do it while avoiding either E128 or
E123 ("continuation line {under,over}-indented for visual indent" respectively)
error from pep8.

Moreover, the wrapping code didn't work correctly anyhow as it only took into
account the length of the parameter list itself and not the total line length,
which should include the function name as well.

So just disable wrapping entirely, long lines shouldn't be a problem anyhow in
auto-generated code.
07e2568
@vadz
SWIG member

Note that the Travis error is an internal g++ compiler error in Octave build, so it is completely unrelated to my changes and seems to be bogus.

@wsfulton
SWIG member

Apart from the special handling of certain named typemaps, like INOUT, this is okay. Can you please change the parameter name checking with what we have in the strongly typed languages. Use makeParameterName() from java.cxx or csharp.cxx to ensure a valid parameter name is created, then you won't need to put in the special handling for empty and duplicate parameter names.

@vadz
SWIG member

I thought about this, but what would be the point, actually? If there is no name, there can't be any documentation for the parameter in the first place. Is having def func(arg1, arg2, arg3): in Python really better than the current def func(*args):? The only benefit I see is that you still know that it takes just 3 arguments, but this doesn't really help when you still don't know what these arguments are...

@wsfulton
SWIG member

All the other parameters (that aren't OUTPUT) will have their names shown, so that is one reason to just replace the OUTPUT with an argx name.

I'd rather not have special name parameter handling. Currently, the implementation allows for a user to use any duplicate name they like and it will work with *args. We may as well helpfully 'fix' this like we do in Java/C#/D etc. If someone wants a decent name, they can provide one and use %apply instead.

Indeed, if there is no parameter then generating argx isn't helpful, but often just one argument in a list does not have a name (such as when it is for future use or deprecated). I think it would then be preferable to have the argument names and documentation appear for the remaining arguments rather than lose all this info just because of one unnamed argument.

@vadz
SWIG member

OK, working on this.

P.S. I'd like to start by factoring out makeParameterName() to Language itself, I don't want to make another copy of it for Python it would be the fifth one (already used in C#, D, Java and Modula3).

@wsfulton
SWIG member

Thanks, yes. Hopefully all of them are identical.

@vadz
SWIG member

I'm testing my changes and while they do seem to work, makeParameterName() doesn't seem to do anything special for INOUT and friends, i.e. they're still used as is in Python if there is only one parameter with such name. Do you really want to keep it like this or should I update the (thankfully unique copy of) makeParameterName() to also use argN for these special parameters for all languages?

@vadz
SWIG member

OK, it took me much longer than I thought it would because of the keyword arguments complications, but finally things seem to work, so I'm pushing the new version.

This still doesn't rename INOUT &c to something sensible as it should, please let me know if you'd like to change this.

Also, while we now have Language::makeParameterName(), there is also, as it turns out, Swig_name_make() which seems to be used for about the same thing in some language modules (notably Ruby which contains code apparently copied from Python and which probably needs to be updated as well -- but this will be left for another day...).

vadz added some commits Aug 16, 2014
@vadz vadz Refactor: move makeParameterName() to common Language base class.
This method was duplicated more or less identically for 4 languages and will
be needed for another one soon, so put it in the base class from which it can
be simply reused instead.

No changes in the program behaviour whatsoever.
9f1af89
@vadz vadz No real changes, just make PYTHON::check_kwargs() const.
This will allow calling it from const methods too.
80a72d5
@vadz vadz Don't always use "*args" for all Python wrapper functions.
Due to what seems like a bug introduced during Python 3 support merge, all the
generated Python functions used the general "*args" signature instead of using
the named parameters when possible.

This happened due to is_primitive_defaultargs() always returning false for the
functions without any default arguments as "value" passed to convertValue()
was NULL in this case and convertValue() always returns false for NULL.

Fix this by checking for value being non-NULL before calling convertValue().

Doing this exposed several problems with the handling of unnamed, duplicate
(happens for parameters called INOUT, for example) or clashing with keywords
parameter names, so the code dealing with them had to be fixed too. Basically
just use makeParameterName() consistently everywhere.
fdc6bbe
@vadz vadz Allow using enum elements as default values for Python functions.
Enum values are just (integer) constants in Python and so can be used as the
function default values just as well as literal numbers, account for this when
checking whether function parameters can be represented in Python.

Also rename is_primitive_defaultargs() to is_representable_as_pyargs() to
describe better what this function does.
15b3690
@vadz
SWIG member

FYI: these commits are also part of PR #170 so if you can merge that one directly, this would be perfect. Otherwise I'll just rebase on latest master once this one is merged, of course. TIA!

@wsfulton
SWIG member

I only want doxygen changes merged in by the doxygen branch and this patch should be committed separately.

Is this patch complete now? I think there was discussion on swig-devel about it not working with some Python command line options??

@vadz
SWIG member

This patch is complete.

The -builtin discussion was about another patch (enums, not submitted yet) and this one should work with it, although I haven't tested it. It would probably be useful to add a test suite run using this switch to Travis...

@vadz
SWIG member

Just FYI, I've tried running the test suite on master with SWIG_FEATURES=-builtin and there are plenty of errors (without this patch), so it's difficult to test if this doesn't break something when -builtin is used. But, frankly, considering the current state, I don't think it matters.

@wsfulton
SWIG member

I have now sorted out the -builtin tests. Travis has two new builds:

SWIGLANG=python SWIG_FEATURES=-builtin
SWIGLANG=python SWIG_FEATURES=-builtin PY3=3

I've been meaning to do this for ages and so have finally done it. I also intend to add in a python -O test-suite

@wsfulton wsfulton merged commit 15b3690 into swig:master Oct 28, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@wsfulton wsfulton added a commit that referenced this pull request Oct 28, 2014
@wsfulton wsfulton Fix autodoc testcase for new named python arguments when using python…
… -builtin

For the changes in #201.
5d71f91
@wsfulton wsfulton added a commit that referenced this pull request Oct 28, 2014
@wsfulton wsfulton Fix when is 'self' used as a parameter name in Python
Fix corner case for variable names called 'self' after merging in patch #201
a6efdb7
@wsfulton
SWIG member

Regarding INOUT, the patch does correct the names. This works:

//%feature("kwargs");

%include <typemaps.i>

%apply int& INOUT {int &one, int &two};
%{
void thing(int& one, int& two) {
  one = one + 1;
  two = two + 2;
}
%}
void thing(int& INOUT, int& INOUT);

But does fail (as it always did) when the kwargs line is uncommented.

@vadz vadz deleted the vadz:py-args branch Oct 28, 2014
@vadz vadz referenced this pull request Oct 28, 2014
Open

Merge doxygen branch #170

@wsfulton
SWIG member

'self' as a parameter name finally fixed in 6029b2f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment