Author mark.dickinson
Recipients Alexander.Belopolsky, akitada, belopolsky, christian.heimes, josm, loewis, mark.dickinson, rhettinger, robertwb, zanella
Date 2010-05-04.16:14:39
SpamBayes Score 2.71864e-11
Marked as misclassified No
Message-id <1272989682.27.0.0624640548133.issue1533@psf.upfronthosting.co.za>
In-reply-to
Content
[Some of the Alexander's questions about procedures aren't really related to this issue;  I've answered those offline.  Here are the answers to the others.]


>>  - 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?

ilow doesn't need to be initialized because the PyArgs_ParseTuple is
guaranteed to either fail or initialize it, and I can't see that the
PyArgs_ParseTuple call is likely to change.  But I admit that the lack
of initialization here also makes me uncomfortable, especially in
combination with the assert that's there.  I might add an
initialization.

Do static code checkers really complain about redundant
initializations?  If anything, it seems like good defensive
programming practice to initialize variables, even
unnecessarily---later refactoring might make those initializations
necessary.

>
>>  - 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.

Okay, fair enough.  I agree it's a matter of taste.  I like the three
separate blocks, one for each argument, especially since the
refcounting semantics are clear:  each block adds exactly one
reference.  But each to his own. :)

>
>>  - 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.

Hmm.  Possibly.  I have an unhealthy and probably irrational aversion
to non-constant static variables, even if the only time that the
variable is changed is at module initialization.

>
>>  - [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.

Yes, possibly not. :)  Partly I made this change because the
assignment 'ilow = zero;' again raises a red flag for me, because it's
not accompanied by a 'Py_INCREF(zero);' as I'd expect it to be.  I
realize that in this case it works out (because ilow is already a
borrowed reference, and we're replacing it with a borrowed reference
to zero), but it's sufficiently unusual that I have to think about it.
 This is personal preference again, I guess.
History
Date User Action Args
2010-05-04 16:14:42mark.dickinsonsetrecipients: + mark.dickinson, loewis, rhettinger, belopolsky, christian.heimes, josm, robertwb, zanella, akitada, Alexander.Belopolsky
2010-05-04 16:14:42mark.dickinsonsetmessageid: <1272989682.27.0.0624640548133.issue1533@psf.upfronthosting.co.za>
2010-05-04 16:14:40mark.dickinsonlinkissue1533 messages
2010-05-04 16:14:39mark.dickinsoncreate