Re: [Python-Dev] [Python-checkins] cpython: Avoid useless "++" at the end of functions
Le jeudi 26 mai 2011 à 08:13 -0400, Eric Smith a écrit :
If you're ever going to add code at the end of these functions, it's unlikely you'll remember that you need to add these increments back in.
You don't have to remember. Test the result of the function, it will not give the expected output. I don't think that you need fuzzing or a complex tool to detect that the new code doesn't behave correctly.
It's a bug waiting to happen
What? It's not a bug. Ading new non-tested code is a bug :-)
I don't see any harm leaving them in. Maybe we should add a comment about why they're done.
It makes Python faster (!) and make silent the Clang Static Analyzer :-) Victor
On 5/26/2011 8:32 AM, Victor Stinner wrote:
Le jeudi 26 mai 2011 à 08:13 -0400, Eric Smith a écrit :
If you're ever going to add code at the end of these functions, it's unlikely you'll remember that you need to add these increments back in.
You don't have to remember. Test the result of the function, it will not give the expected output. I don't think that you need fuzzing or a complex tool to detect that the new code doesn't behave correctly.
It's a bug waiting to happen
What? It's not a bug. Ading new non-tested code is a bug :-)
True. But assuming all code additions will have 100% branch coverage in the C code is foolish.
I don't see any harm leaving them in. Maybe we should add a comment about why they're done.
It makes Python faster (!)
I doubt that.
and make silent the Clang Static Analyzer :-)
I care less about that than maintainability and future-proofing.
On 26 May, 2011, at 16:10, Eric Smith wrote:
and make silent the Clang Static Analyzer :-)
I care less about that than maintainability and future-proofing.
Have to looked at the patch? The patch and resulting code look sane to me, and if anything at most of the updated segments look cleaner after the patch. Ronald
On 5/26/2011 10:34 AM, Ronald Oussoren wrote:
On 26 May, 2011, at 16:10, Eric Smith wrote:
and make silent the Clang Static Analyzer :-)
I care less about that than maintainability and future-proofing.
Have to looked at the patch? The patch and resulting code look sane to me, and if anything at most of the updated segments look cleaner after the patch.
I have looked at it. I think the code was better before the patch. If I were looking at this, I'd have to wonder why the pointer was incremented everywhere else, but not here. This is especially true when the changed code isn't particularly near the end of the function.
On Thu, May 26, 2011 at 11:26 AM, Eric Smith <eric@trueblade.com> wrote: ..
Have to looked at the patch? The patch and resulting code look sane to me, and if anything at most of the updated segments look cleaner after the patch.
I have looked at it. I think the code was better before the patch. If I were looking at this, I'd have to wonder why the pointer was incremented everywhere else, but not here. This is especially true when the changed code isn't particularly near the end of the function.
+1 To me, *p++ = c is an idiomatic way to fill the buffer. I prefer to think of p as the state of the stream for which adding a character is impossible without advancing the state. Seeing *p = c will definitely make me pause and think whether or not it is a bug.
On 5/26/2011 10:34 AM, Ronald Oussoren wrote:
On 26 May, 2011, at 16:10, Eric Smith wrote:
and make silent the Clang Static Analyzer :-)
I care less about that than maintainability and future-proofing.
Have to looked at the patch? The patch and resulting code look sane to me, and if anything at most of the updated segments look cleaner after the patch.
Lets assume that the function currently does what it is supposed to do, as verified by tests. Then adding an unneeded increment in case the function is redefined in the future so that it needs more code strikes me as YAGNI. Certainly, reading it today with an unused increment suggests to me that something is missing that would use the incremented value. This strike me as different from adding a comma at the end of a Python sequence display. -- Terry Jan Reedy
On Thu, May 26, 2011 at 10:59 AM, Terry Reedy <tjreedy@udel.edu> wrote:
On 5/26/2011 10:34 AM, Ronald Oussoren wrote:
On 26 May, 2011, at 16:10, Eric Smith wrote:
and make silent the Clang Static Analyzer :-)
I care less about that than maintainability and future-proofing.
Have to looked at the patch? The patch and resulting code look sane to me, and if anything at most of the updated segments look cleaner after the patch.
Lets assume that the function currently does what it is supposed to do, as verified by tests. Then adding an unneeded increment in case the function is redefined in the future so that it needs more code strikes me as YAGNI. Certainly, reading it today with an unused increment suggests to me that something is missing that would use the incremented value. This strike me as different from adding a comma at the end of a Python sequence display.
Sorry to butt in here, but I agree with Eric that it was better before. There is a common idiom, *pointer++ = <something>, and whenever you see that you know that you are appending something to an output buffer. Perhaps the most important idea here is that this maintains the *invariant* "pointer points just after the last thing in the buffer". Always maintaining the invariant is better than trying to micro-optimize things so as to avoid updating dead values. The compiler is better at that. -- --Guido van Rossum (python.org/~guido)
On 5/26/2011 2:08 PM, Guido van Rossum wrote:
Sorry to butt in here, but I agree with Eric that it was better before. There is a common idiom, *pointer++ =<something>, and whenever you see that you know that you are appending something to an output buffer. Perhaps the most important idea here is that this maintains the *invariant* "pointer points just after the last thing in the buffer". Always maintaining the invariant is better than trying to micro-optimize things so as to avoid updating dead values. The compiler is better at that.
This explanation makes sense (more than Eric's version of perhaps the same thing ;-). http://bugs.python.org/issue12188 "A condensed version of the above added to PEP 7 would help new developers see the usage as local idiom rather than style bug." Terry J. Reedy
2011/5/26 Terry Reedy <tjreedy@udel.edu>:
On 5/26/2011 2:08 PM, Guido van Rossum wrote:
Sorry to butt in here, but I agree with Eric that it was better before. There is a common idiom, *pointer++ =<something>, and whenever you see that you know that you are appending something to an output buffer. Perhaps the most important idea here is that this maintains the *invariant* "pointer points just after the last thing in the buffer". Always maintaining the invariant is better than trying to micro-optimize things so as to avoid updating dead values. The compiler is better at that.
This explanation makes sense (more than Eric's version of perhaps the same thing ;-).
http://bugs.python.org/issue12188 "A condensed version of the above added to PEP 7 would help new developers see the usage as local idiom rather than style bug."
I think a more general formulation would be: "Idiomatic code is more important than making static analyzers happy." -- Regards, Benjamin
2011/5/26 Victor Stinner <victor.stinner@haypocalc.com>:
Le jeudi 26 mai 2011 à 08:13 -0400, Eric Smith a écrit :
If you're ever going to add code at the end of these functions, it's unlikely you'll remember that you need to add these increments back in.
You don't have to remember. Test the result of the function, it will not give the expected output. I don't think that you need fuzzing or a complex tool to detect that the new code doesn't behave correctly.
It's a bug waiting to happen
What? It's not a bug. Ading new non-tested code is a bug :-)
I don't see any harm leaving them in. Maybe we should add a comment about why they're done.
It makes Python faster (!) and make silent the Clang Static Analyzer :-)
Surely, GCC can optimize that out. -- Regards, Benjamin
participants (7)
-
Alexander Belopolsky -
Benjamin Peterson -
Eric Smith -
Guido van Rossum -
Ronald Oussoren -
Terry Reedy -
Victor Stinner