Seems like a good idea to tighten it up. If a style guide is going to say "you can either do X or not do X", it might as well not mention X at all. :-) -- Greg

01.06.17 09:36, Benjamin Peterson пише:

Modern GCC can defend against these kinds of problems. If I introduce a "goto fail" bug somewhere in Python, I get a nice warning: ../Objects/abstract.c: In function ‘PyObject_Type’: ../Objects/abstract.c:35:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] if (o == NULL) ^~ ../Objects/abstract.c:37:9: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’ 2 + 3; ^

This is not to say that simply bracing everything isn't the right way to go.

Actually a bug with misleadingly indented statement in CPython sources was fixed just few months ago (thanks to modern GCC).

the 'goto fail' bug is a somewhat extreme reminder for why such braces are a good idea (as Victor said) - https://www.imperialviolet.org/2014/02/22/applebug.html On Wed, May 31, 2017 at 6:25 PM Paul Moore wrote:

On 31 May 2017 at 15:11, Victor Stinner wrote:

...

So I would suggest to modify the PEP 7 to *always* require braces for if.

I would also suggest to require braces on "for(...) { ... }" and "while(...) { ... }". But only if the code has to be modified, not only to update the coding style.

Changes which are pure coding style changes should be rejected to avoid conflicts with other pending pull requests and "code churn".

As a practical compromise, I'd argue that if you're changing *existing* code, you should retain the current style. For *new* code, always add braces. So, changing

if (func == NULL) return NULL; /* further code */

to

if (func1 == NULL) return NULL; /* further code */

you would leave out the braces. But changing

/* Code using func */

to

if (func == NULL) { return NULL; } /* Code using func */

you include the braces, because it's new code.

I'm not against making the PEP mandate braces, but I don't think it's necessary to resolve the situation you describe as having triggered the suggestion :-)

Paul _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/orenmn%40gmail.com

-- -Oren

I interpret the PEP as saying that you should use braces everywhere but not to add them in code that you're not modifying otherwise. (I.e. don't go on a brace-adding rampage.) If author and reviewer of a PR disagree I would go with "add braces" since that's clearly the PEP's preference. This is C code. We should play it safe. On Wed, May 31, 2017 at 9:02 AM, Serhiy Storchaka wrote:

31.05.17 17:11, Victor Stinner пише:

I have a question on the CPython coding code for C code, the PEP 7:

...

https://www.python.org/dev/peps/pep-0007/

""" Code structure: (...); braces are strongly preferred but may be omitted where C permits, and they should be formatted as shown:

if (mro != NULL) { ... } else { ... } """

This section was updated after the creation of CPython in 1991, so as you may expect, a lot of existing C code doesn't respect this coding style. I'm trying to slowly add "missing" braces and indent as the example *when I have to modify existing code*.

The problem is the "strongly preferred" and "omitted where C permits" part. I would like to make the PEP 7 more explicit on that part since in each review, I disagree with Serhiy who wants to omit them. Example:

if (func == NULL) return NULL;

versus

if (func == NULL) { return NULL; }

I now prefer the version with braces. It's more verbose, but it prevents bugs when the code is modified later for whatever reasons. IMHO it also makes the code easily to read and to understand.

So I would suggest to modify the PEP 7 to *always* require braces for if.

I would also suggest to require braces on "for(...) { ... }" and "while(...) { ... }". But only if the code has to be modified, not only to update the coding style.

Thank you for opening this discussion Victor. I was going to open it myself.

I strongly prefer using braces around "for" and "while" bodies. I dislike unconditional adding braces, but after changing PEP 7 I became always adding them in my code and asking this in reviewed code. I even made Argument Clinic generating braces around gotos. But when I asked Barry to add braces around simple statements to satisfy PEP 7 requirements, he pointed out that braces are only "strongly preferred". Since that time I allowed myself to left braces out in cases when they look cluttering.

I think that PEP 7 should be more explicit about cases in what braces are required and in what they are optional. I prefer to make them optional (or even make the variant without braces preferable) in case of "if" (without "else") or "case" body containing only the single "goto", "break", "continue" or simple "return". Especially if it follows by an empty line or closing brace. Adding braces in these cases decreases readability IMHO. This is one peculiarity for which many people like (or hate) Python.

_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/guido% 40python.org

-- --Guido van Rossum (python.org/~guido)

2017-05-31 19:27 GMT+02:00 Guido van Rossum :

I interpret the PEP (...)

Right, the phrasing requires to "interpret" it :-)

(...) as saying that you should use braces everywhere but not to add them in code that you're not modifying otherwise. (I.e. don't go on a brace-adding rampage.) If author and reviewer of a PR disagree I would go with "add braces" since that's clearly the PEP's preference. This is C code. We should play it safe.

Would someone be nice enough to try to rephrase the PEP 7 to explain that? Just to avoid further boring discussion on the C coding style... Victor

https://github.com/python/peps/pull/280/files On Jun 01, 2017, at 09:08 PM, Brett Cannon wrote:

If you create an issue at github.com/python/peps and assign it to me I will get to it someday. :)

On Jun 03, 2017, at 07:25 PM, Serhiy Storchaka wrote:

But the latter example continuation lines are intended at the same level as the following block of code. I propose to make exception for that case and allow moving an open brace to the start of the next line.

if (type->tp_dictoffset != 0 && base->tp_dictoffset == 0 && type->tp_dictoffset == b_size && (size_t)t_size == b_size + sizeof(PyObject *)) { return 0; /* "Forgive" adding a __dict__ only */ }

Agreed! https://github.com/python/peps/issues/283 https://github.com/python/peps/pull/284 Cheers, -Barry

On Mon, Jun 5, 2017 at 12:41 AM, Serhiy Storchaka wrote:

Barry and Victor prefer moving a brace on a new line in all multiline conditional cases. I think that it should be done only when the condition continuation lines and the following block of the code have the same indentation (as in the example above), and the following code is enough readable:

if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, "invalid escape sequence '\\%c'", *first_invalid_escape) < 0) { Py_DECREF(result); return NULL; }

What other core developers think about this?

Wow, this discussion takes me back. Glad I don't have to check out comp.lang.c to get my brace placement fix. <wink> Skip

On Jun 05, 2017, at 07:00 AM, Skip Montanaro wrote:

Wow, this discussion takes me back. Glad I don't have to check out comp.lang.c to get my brace placement fix. <wink>

Life is better without braces! -Barry

On Jun 05, 2017, at 08:41 AM, Serhiy Storchaka wrote:

the example above), and the following code is enough readable:

if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, "invalid escape sequence '\\%c'", *first_invalid_escape) < 0) { Py_DECREF(result); return NULL; }

My rationale for placing the opening brace on a separate line, indented to under the `if` instead of hanging is that it's easier to miss the opening brace in the example you posted above. Visually I (we?) tend to have a harder time recognizing characters sitting way out to the right. On Jun 05, 2017, at 08:19 AM, Ethan Furman wrote:

I would format that as:

if (PyErr_WarnFormat( PyExc_DeprecationWarning, 1, "invalid escape sequence '\\%c'", *first_invalid_escape) < 0) { Py_DECREF(result); return NULL; }

In this case I'd *still* indent the opening brace to under the `if`. The mismatched indentation between the open and close braces is jarring to me.

- having all the arguments on separate lines means - the function and first argument don't get run together - it's easy to pick out the individual arguments

That's fine with me, but so is hanging the arguments, so I'd tend to leave this up to the individual devs.

- having the opening brace on its own line means - a little extra white space to buffer the condition and the body - it's easier to read the function name and then drop down to the body

Agreed with the rationale for the open brace being on a separate line, but did you mean to indent the opening and closing braces to different levels? Cheers, -Barry

On 06/06/2017 05:30 PM, Barry Warsaw wrote:

On Jun 05, 2017, at 08:19 AM, Ethan Furman wrote:

...

I would format that as:

if (PyErr_WarnFormat( PyExc_DeprecationWarning, 1, "invalid escape sequence '\\%c'", *first_invalid_escape) < 0) { Py_DECREF(result); return NULL; }

In this case I'd *still* indent the opening brace to under the `if`. The mismatched indentation between the open and close braces is jarring to me.

...

- having all the arguments on separate lines means - the function and first argument don't get run together - it's easy to pick out the individual arguments

That's fine with me, but so is hanging the arguments, so I'd tend to leave this up to the individual devs.

...

- having the opening brace on its own line means - a little extra white space to buffer the condition and the body - it's easier to read the function name and then drop down to the body

Agreed with the rationale for the open brace being on a separate line, but did you mean to indent the opening and closing braces to different levels?

It's what I see. Left to my own devices I would leave the opening brace where it is and indent the closing brace to match. That way when I see code at the same level as the opening `if` I know I'm out of that block. -- ~Ethan~