Reformat threading macros in _tkinter #25026
Conversation
Combining conditional and non-conditional statements in one line is quite confusing, and is being flagged as misleading indentation by static analyzers. Style-only changes are generally froned upon in CPython. But here, the code might be interpreted differently by humans than by machines, so it warrants an exception.
The thread-related macros in _tkinter are slit to have one statement per line and indented to show structure. (Deliberately breaking a PEP7 rule, single-statement `if` blocks are kept on one line in the macros to limit noisy "\" lines. The bodies are bracketed to make the code flow clear.)
|
Your changes definitely make the code look cleaner. +1. |
|
This PR is stale because it has been open for 30 days with no activity. |
|
The commit 6ec259c is certainly better.
Given that it is defining a macro, and we are able to follow (even if it is takes slight extra effort, cases like this exist in many places), I think, we should do style changes along with functional changes so that history will be easier to follow. -1 to this change. If no other core-dev comes in support and merges this PR, we could close this PR. Thank you! |
| { \ | ||
| PyThreadState *tstate = PyThreadState_Get(); \ | ||
| Py_BEGIN_ALLOW_THREADS \ | ||
| if(tcl_lock) { PyThread_acquire_lock(tcl_lock, 1); } \ |
There was a problem hiding this comment.
Add a space after if. And I am not sure that braces add readability.
PEP7 requires adding braces and writing it in three lines
if (tcl_lock) {
PyThread_acquire_lock(tcl_lock, 1);
}
but I think that for macros we can make exception and write it in one line:
if (tcl_lock) PyThread_acquire_lock(tcl_lock, 1);
If you strongly prefer writing it in more verbose way, I suggest to align backslashes:
#define ENTER_TCL \
{ \
PyThreadState *tstate = PyThreadState_Get(); \
Py_BEGIN_ALLOW_THREADS \
if (tcl_lock) { \
PyThread_acquire_lock(tcl_lock, 1); \
} \
tcl_tstate = tstate;
I noticed this too, but I didn't go into details about the inconsistency. Thanks for picking. I am fine with the changes you are proposing. |
github-actions
bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
|
Superseded by GH-26244. |
I got a static analyzer report about misleading indentation in
_tkinter. The indentation really is misleading, the worst example beingENTER_PYTHON:That last line combines:
ifis one line above,if.I'm well aware that style-only changes are generally frowned upon in CPython. I propose making an exception here, since the code might actually be interpreted differently by humans than by machines.
Since I'm touching this code, I went ahead and reformatted the macros more thoroughly. If that's not wanted, I can drop the second commit.
(Deliberately breaking a PEP7 rule, I kept single-statement
ifblocks on one line in macros to limit noisy\lines. The bodies are bracketed to make the code flow clear.)