This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author belopolsky
Recipients Alexander.Belopolsky, akitada, belopolsky, christian.heimes, josm, loewis, mark.dickinson, rhettinger, robertwb, zanella
Date 2010-05-04.15:11:15
SpamBayes Score 1.2989609e-14
Marked as misclassified No
Message-id <1272985879.73.0.568471836882.issue1533@psf.upfronthosting.co.za>
In-reply-to
Content
On Tue, May 4, 2010 at 8:34 AM, Mark Dickinson <report@bugs.python.org> wrote:
..
> I took the liberty of messing with your patch slightly;  I didn't want
> to ask you to make further changes since the patch was fine, and my
> messing was mostly just to satisfy my own fussiness (only the first
> two items were really necessary):
>

Thank you for doing it.  The patch is good to go, questions and comments below are just for my education.

> Minor updates:
>  - add Misc/NEWS entry

What is the standard practice on this?  I thought Misc/NEWS entry was similar to commit message and would be written by a committer.  What are the guidelines for writing such entries?  I see that some entries simply repeat the title of the issues while others got into greater details.

>  - remove trailing whitespace and fix spaces that should have been
> a tab

I've looked at my patch through cat -et and couldn't find any tab/space issues.  I did notice one empty line with a single space that you removed.  Do you use an automated checking tool for this?  I just did grep " $".  BTW, the current trunk version (r80752) of Python/bltinmodule.c has two lines with trailing white space:

$ cat -n Python/bltinmodule.c | grep " $" | cat -et
   641^I^I^IPyErr_SetString(PyExc_TypeError, $
  2966^I        $


>  - added some extra tests to check all possible combinations of bad
>   arguments, purely to check for refcounting problems

With arguments processing segregated in a common function, I am not sure  whether additional tests actually increase coverage.  This brings a question: what is the recommended way to produce coverage statistics for  C modules?  regrtest.py --coverage seems to be for python modules only. 

>  - initialize low to NULL, to match the Py_XDECREF(low) (could change
>   that Py_XDECREF to Py_DECREF instead, but the code's more resistant
>   to random refactorings this way --- see next item :)

Good catch.  Wouldn't the same argument apply to ilow?  Wouldn't static code checkers complain about redundant initialization?

>  - randomly refactor:  regroup blocks for ease of reading

I actually disagree that your regrouping makes the code clearer.  In my version, all idiosyncratic argument processing is done with borrowed references first followed by common processing in three similar blocks.  This, however, is purely matter of taste.  Note that I considered changing i* names to raw* names, but decided not to introduce more changes than necessary.  In your grouping, however, the similarity of variable names is more of an issue.  This said, I don't really have any problem with your choice.

>  - don't do PyLong_FromLong(1) until it's needed ('zero' is different,
>   since it's always used in the non-error case)

Yes, I considered that. A further micro-optimization would be to initialize a static variable in module initialization and reuse it in get_len_of_range_longs as well.  I decided to put it next to zero instead to simplify the logic.

>  - [micro-optimization]: don't pass a known zero value to
>   get_range_long_argument

Is it really worth it?  Default start is probably not that common in case of long arguments.


> Any objections or comments?  Can I apply this version of the patch?
>

The above are not objections.  Please apply this version of the patch.
History
Date User Action Args
2010-05-04 15:11:20belopolskysetrecipients: + belopolsky, loewis, rhettinger, mark.dickinson, christian.heimes, josm, robertwb, zanella, akitada, Alexander.Belopolsky
2010-05-04 15:11:19belopolskysetmessageid: <1272985879.73.0.568471836882.issue1533@psf.upfronthosting.co.za>
2010-05-04 15:11:17belopolskylinkissue1533 messages
2010-05-04 15:11:16belopolskycreate