Re: [Python-Dev] cpython: Remove some extraneous parentheses and swap the comparison order to
On 06/07/11 05:20, brett.cannon wrote:
http://hg.python.org/cpython/rev/fc282e375703 changeset: 70695:fc282e375703 user: Brett Cannon <brett@python.org> date: Mon Jun 06 20:20:36 2011 -0700 summary: Remove some extraneous parentheses and swap the comparison order to prevent accidental assignment.
Silences a warning from LLVM/clang 2.9.
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special? I think that another developer even got told off once for these kinds of comparisons. I hope the Clang warning is only about the parentheses. Georg
files: Modules/arraymodule.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c --- a/Modules/arraymodule.c +++ b/Modules/arraymodule.c @@ -2091,7 +2091,7 @@ if (len == 0) { return PyUnicode_FromFormat("array('%c')", (int)typecode); } - if ((typecode == 'u')) + if ('u' == typecode) v = array_tounicode(a, NULL); else v = array_tolist(a, NULL);
On Tue, 07 Jun 2011 08:57:10 +0200 Georg Brandl <g.brandl@gmx.net> wrote:
On 06/07/11 05:20, brett.cannon wrote:
http://hg.python.org/cpython/rev/fc282e375703 changeset: 70695:fc282e375703 user: Brett Cannon <brett@python.org> date: Mon Jun 06 20:20:36 2011 -0700 summary: Remove some extraneous parentheses and swap the comparison order to prevent accidental assignment.
Silences a warning from LLVM/clang 2.9.
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
Agreed. Either we do it wholesale (I find these "reversed" comparisons a bit ugly myself) or there's no point doing it on a single occurrence. Regards Antoine.
Georg Brandl wrote:
On 06/07/11 05:20, brett.cannon wrote:
http://hg.python.org/cpython/rev/fc282e375703 changeset: 70695:fc282e375703 user: Brett Cannon <brett@python.org> date: Mon Jun 06 20:20:36 2011 -0700 summary: Remove some extraneous parentheses and swap the comparison order to prevent accidental assignment.
Silences a warning from LLVM/clang 2.9.
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
I think that another developer even got told off once for these kinds of comparisons.
I hope the Clang warning is only about the parentheses.
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison. If clang warns about this, clang needs to be fixed, not our C code :-)
Georg
files: Modules/arraymodule.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c --- a/Modules/arraymodule.c +++ b/Modules/arraymodule.c @@ -2091,7 +2091,7 @@ if (len == 0) { return PyUnicode_FromFormat("array('%c')", (int)typecode); } - if ((typecode == 'u')) + if ('u' == typecode) v = array_tounicode(a, NULL); else v = array_tolist(a, NULL);
_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/mal%40egenix.com
-- Marc-Andre Lemburg eGenix.com Professional Python Services directly from the Source (#1, Jun 07 2011)
Python/Zope Consulting and Support ... http://www.egenix.com/ mxODBC.Zope.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
2011-05-23: Released eGenix mx Base 3.2.0 http://python.egenix.com/ 2011-05-25: Released mxODBC 3.1.1 http://python.egenix.com/ 2011-06-20: EuroPython 2011, Florence, Italy 13 days to go ::: Try our new mxODBC.Connect Python Database Interface for free ! :::: eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 http://www.egenix.com/company/contact/
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
I think that another developer even got told off once for these kinds of comparisons.
I hope the Clang warning is only about the parentheses.
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison.
If clang warns about this, clang needs to be fixed, not our C code :-)
+1 Placing the constant first in a comparison is a fundamental style issue. Personally I also don't like doing that, but whatever way is chosen must be consistent. It's definitely wrong to change this in a single place. We have PEP-7 for these things! AFAIK, Clang doesn't produce a warning for this, at least without special static-analysis warning levels. Eli
On Tue, Jun 7, 2011 at 2:47 AM, Eli Bendersky <eliben@gmail.com> wrote:
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
I think that another developer even got told off once for these kinds of comparisons.
I hope the Clang warning is only about the parentheses.
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison.
If clang warns about this, clang needs to be fixed, not our C code :-)
+1
Placing the constant first in a comparison is a fundamental style issue. Personally I also don't like doing that, but whatever way is chosen must be consistent. It's definitely wrong to change this in a single place. We have PEP-7 for these things!
Right. I personally really despise putting the constant first.
AFAIK, Clang doesn't produce a warning for this, at least without special static-analysis warning levels.
CLang shouldn't force our hand here. -- --Guido van Rossum (python.org/~guido)
..
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison.
I appear to be in the minority here, but this particular example does not strike me as egregiously unreadable. To the contrary, by bringing the constant to the front, this form saves me from having to read to the end of the line. The same mental economy appears when constants are brought to the front of chained if-elif cases in Python: if 'a' == typecode: .. elif 'b' == typecode: .. is slightly more readable than the more traditional alternative. Probably because I can mentally ignore the "== typecode" part and see the switch structure more clearly. Either way, I don't see a big issue here and I would keep "len == 0" intact even if I reordered typecode == 'u' as Brett did. The only consistency that I would enforce is to use the same order in the chained if-elif cases, but otherwise this should be left to the discretion of the author.
On Tue, 07 Jun 2011 17:42:17 -0400, Alexander Belopolsky <alexander.belopolsky@gmail.com> wrote:
..
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison.
I appear to be in the minority here, but this particular example does not strike me as egregiously unreadable. To the contrary, by bringing the constant to the front, this form saves me from having to read to the end of the line. The same mental economy appears when constants are brought to the front of chained if-elif cases in Python:
if 'a' == typecode: .. elif 'b' == typecode: ..
is slightly more readable than the more traditional alternative. Probably because I can mentally ignore the "== typecode" part and see the switch structure more clearly.
I don't do much C coding, so I don't have the right to an opinion on that (FWIW, I find constant-first jarring). But I'd hate to see the above in python code. The fact that you like it because it makes it easier to read as a switch-like statement should instead tell you that it is time to refactor the code. -- R. David Murray http://www.bitdance.com
On Tue, 2011-06-07 at 11:03 +0200, M.-A. Lemburg wrote:
Georg Brandl wrote:
On 06/07/11 05:20, brett.cannon wrote:
http://hg.python.org/cpython/rev/fc282e375703 changeset: 70695:fc282e375703 user: Brett Cannon <brett@python.org> date: Mon Jun 06 20:20:36 2011 -0700 summary: Remove some extraneous parentheses and swap the comparison order to prevent accidental assignment.
Silences a warning from LLVM/clang 2.9.
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
I think that another developer even got told off once for these kinds of comparisons.
I hope the Clang warning is only about the parentheses.
I agree with Georg: "if ('u' == typecode)" is not well readable, since you usually put the variable part on the left and the constant part on the right of an equal comparison.
[FWIW, I'm one of the reprobates that likes to put the constant on the LHS when I'm coding in C, but I see I'm in the minority here] I know that this style is unpopular, but if it helps, try mentally pronouncing "==" in C as "is the value of". In this example, when I read that line, my mind is thinking: "if 'u' is the value of typecode" After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :) [snip] Hope this is helpful Dave
On 6/7/2011 5:35 PM, David Malcolm wrote:
I know that this style is unpopular, but if it helps, try mentally pronouncing "==" in C as "is the value of".
In this example, when I read that line, my mind is thinking:
"if 'u' is the value of typecode"
After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :)
Whereas I read it as 'has the value' (or just 'is' ;=). Not being tempted to reverse in Python is one advantage of not having assignment expressions. -- Terry Jan Reedy
Terry Reedy wrote:
On 6/7/2011 5:35 PM, David Malcolm wrote:
I know that this style is unpopular, but if it helps, try mentally pronouncing "==" in C as "is the value of".
In this example, when I read that line, my mind is thinking:
"if 'u' is the value of typecode"
After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :)
Whereas I read it as 'has the value' (or just 'is' ;=).
Am I the only one who reads == as "equals"? -- Steven
On Wed, Jun 8, 2011 at 7:04 AM, Steven D'Aprano <steve@pearwood.info> wrote: ..
Whereas I read it as 'has the value' (or just 'is' ;=).
Am I the only one who reads == as "equals"?
If you are, you are the only one who reads it correctly. Consider
a = 2 a == 2.0 True
On Wed, 08 Jun 2011 21:04:48 +1000, Steven D'Aprano <steve@pearwood.info> wrote:
Terry Reedy wrote:
On 6/7/2011 5:35 PM, David Malcolm wrote:
I know that this style is unpopular, but if it helps, try mentally pronouncing "==" in C as "is the value of".
In this example, when I read that line, my mind is thinking:
"if 'u' is the value of typecode"
After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :)
Whereas I read it as 'has the value' (or just 'is' ;=).
Am I the only one who reads == as "equals"?
No :) Especially considering that Python actually has an 'is' operator.... -- R. David Murray http://www.bitdance.com
On Wed, Jun 8, 2011 at 7:35 AM, David Malcolm <dmalcolm@redhat.com> wrote:
After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :)
I actually thought Brett's rationale in the checkin comment was reasonable (if you get in the habit of putting constants on the left, then the classic "'=' instead of '=='" typo is a compiler error instead of a reassignment). Call it a +0 in favour of letting people put constants on the left in C code if they prefer it that way, so long as any given if/elif chain is consistent in the style it uses. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Wed, Jun 8, 2011 at 8:12 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Wed, Jun 8, 2011 at 7:35 AM, David Malcolm <dmalcolm@redhat.com> wrote:
After ~12 years of doing this, it comes naturally. I appreciate that this may come across as weird though :)
I actually thought Brett's rationale in the checkin comment was reasonable (if you get in the habit of putting constants on the left, then the classic "'=' instead of '=='" typo is a compiler error instead of a reassignment).
I really like consistency across the code base. I really don't like constant-on-the-left, and it's basically not used in the current codebase. Please be consistent and don't start using it.
Call it a +0 in favour of letting people put constants on the left in C code if they prefer it that way, so long as any given if/elif chain is consistent in the style it uses.
Sorry, I give it a -1. (I'd like to be able to read the codebase still... :-) -- --Guido van Rossum (python.org/~guido)
On Mon, Jun 6, 2011 at 23:57, Georg Brandl <g.brandl@gmx.net> wrote:
On 06/07/11 05:20, brett.cannon wrote:
http://hg.python.org/cpython/rev/fc282e375703 changeset: 70695:fc282e375703 user: Brett Cannon <brett@python.org> date: Mon Jun 06 20:20:36 2011 -0700 summary: Remove some extraneous parentheses and swap the comparison order to prevent accidental assignment.
Silences a warning from LLVM/clang 2.9.
Swapping the comparison order here seems a bit inconsistent to me. There are lots of others around (e.g. "len == 0" in the patch context below). Why is this one so special?
Old habit on how to do comparisons in C. Because C treats assignment as an expression it means comparisons can accidentally become an assignment if you accidentally leave out an = sign. By reversing this order it is simply not possible to have that silent bug and instead you would get a compiler error about trying to assign to a constant. I'll revert that part of the change.
I think that another developer even got told off once for these kinds of comparisons.
I hope the Clang warning is only about the parentheses.
Yes, Clang only warned about the parentheses. -Brett
Georg
files: Modules/arraymodule.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c --- a/Modules/arraymodule.c +++ b/Modules/arraymodule.c @@ -2091,7 +2091,7 @@ if (len == 0) { return PyUnicode_FromFormat("array('%c')", (int)typecode); } - if ((typecode == 'u')) + if ('u' == typecode) v = array_tounicode(a, NULL); else v = array_tolist(a, NULL);
_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/brett%40python.org
participants (12)
-
Alexander Belopolsky -
Antoine Pitrou -
Brett Cannon -
David Malcolm -
Eli Bendersky -
Georg Brandl -
Guido van Rossum -
M.-A. Lemburg -
Nick Coghlan -
R. David Murray -
Steven D'Aprano -
Terry Reedy