This is a fix for [ 1283289 ] PyArg_ParseTupleAndKeywords gives misleading error message.  It also yields a 10-15% decrease in cpu usage, depending on the number of arguments.

Patched code compiles and passes test suite, and looks cleaner than the
code it replaces. It works on the cases the bug report.

There should be unit tests added to Lib/test/test_getargs2; I've skimmed
the tests in there and can't see any that test combinations of kw, tuple
and positional arguments. The patch replaces significant chunks of arg
parsing code, so I'd be more convinced of its correctness if there were
some unit tests. Certainly there should be at least one test for the
bug the patch fixes.

The code is 99% written in the prevailing style, but:
Source contains several over-long lines, and non-standard comments, ie
you should use
 /* XXX something .. */
and not
 /* ??? something .. ??? */
Also there are a few minor spacing inconsistencies which would be nice
to tidy up.

The diff is one level too high up - it contains the directory Python2.5/
It applies cleanly to the trunk though, it just makes it slightly more difficult
to apply the patch.

The original bug report requests a post to python-dev describing the new
error messages. I suggest this post includes all cases where
the new code produces a different message than the old.

I suggest to proceed:
1. Add unit tests, both for the bug-fix and for existing functionality
   which has been modified
2. Fix up comments and long lines
3. Check with python-dev about new error messages

File Added: getargs_1.patch

File Added: _testcapimodule.patch

File Added: test_getargs2.patch

The patch to the test code looks fine, although there are some style issues.  Some lines over 80 columns and there needs to be spaces around = and other binary operators.

I need to review the C code changes, but this patch looks interesting.  

Roger, in the future, can you make a single patch with all the files.  It's easier to download and apply one patch with multiple files than 3 diff patches.

Can you review the patch and commit it for 2.6? A patch which cleans up
the code *and* make it faster is always a good idea. :)

Christian,

Could you clean this patch up?  Specifically:

 * Put everything into one patch
 * Eliminate unnecessary changes (changing variable name or whitespace)
 * Conform to the style in the file
 * Verify all the tests run with regrtest.py -u all  when built
--without-pydebug
 * Verify it runs faster

I did some cleanup (style, var names, <80 chars per line) and combined
the patch set into a single patch. The regression tests are passing for
a pydebug build. I'm too busy to profile and test the patch with a
vanilla Python right now.

Pybench is showing a small speedup (first run with patch, second run
without patch): 26498ms 25920ms   +2.2% 28000ms 26888ms   +4.1%

I looked over the new patch Christian uploaded and I think I understand
what's going on.  I didn't do a through comparison, but if all the tests
pass, I think that's good enough.  Good work!

Here are the issues I would like to see fixed before check in (all small
and easy):
 * Fix the format nits.  There were missing spaces around = and ==.
 * Remove the comment that says:
+				 * XXX TypeError doesn't seem right for this,
+				 * maybe create an ArgumentError ???
+				 * XXX Arg positions currently reported as
+				 * 1-based, contrary to most things in
+				 * python ???

    The first line of that comment should stay.
 * Expand the comment that says: Maybe skip this in debug build ?
    Add a XXX or TODO and explain why.  I think it's just that it would
give extra testing.  I'm not sure it's worth it since that would be
added code, but it's a valid question.

 * Verify that it's faster by compiling python in a release build (ie,
optimized, not debug with assertions) and do something that would
trivially execute the code.  For example:

  * ''.split()
  * ''.split('', 1)
  * (1,).index(1)
  * (1,).index(1, 0, 1)

That might be a decent set.  I just grepped for PyArg_ParseTuple
Objects/*.c and found some common cases where this code would be used. 
It would be great to add the timing info from the test cases above (as
well as the pybench results) to the check in message.

 * In the test do a single import at the top of the file.
 * Use raise Error('') instead of raise Error, '' in the test.  Actually
instead of raising an exception, call self.fail(error_message)

> * Fix the format nits.  There were missing spaces around = and ==.

Oh, I missed the macro above the function. :]

> * Verify that it's faster by compiling python in a release build 

It's roughly the same speed. :/ The variation between timeit runs is
quite high on my machine. The new code is neither significant faster nor
noticeable slower.

./python Lib/timeit.py "for i in xrange(100): ''.split(); ''.split(' ',
1); (1,).index(1); (1,).index(1, 0, 1)"
1000 loops, best of 3: 342 usec per loop

The timing distributes about +-15usec with 400+ every now and then.

./python Lib/timeit.py -s "def func(a, b, c=None, d=None, e=None): pass"
"for i in xrange(100): func(a=1, b=2); func(b=1, a=2, d=3, c=4, e=5)"

This benchmark shows a noticeable speedup. It's about 205usec with patch
and 225usec without patch.

On Tue, Feb 26, 2008 at 12:51 AM, Christian Heimes
<report@bugs.python.org> wrote:
>
>  > * Verify that it's faster by compiling python in a release build
>
>  It's roughly the same speed. :/ The variation between timeit runs is
>  quite high on my machine. The new code is neither significant faster nor
>  noticeable slower.

Use your best judgment.  I haven't looked at the resulting code, only
the patch so it's hard for me to know for certain.  It appears the
patch simplifies the code, so even if there is no speed benefit, there
may be a maintainability benefit.  It's up to you.

I've applied the patch in r61086.