Message 104953 - Python tracker (original) (raw)

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