Skip to content

Conversation

@picnixz
Copy link
Member

@picnixzpicnixz commented Jun 4, 2024

For the review, it's easier to check the diff by hiding the whitespace changes (also, could someone add the skip news label, please?)

@picnixzpicnixz requested a review from sobolevnJune 14, 2024 09:28
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff could be minimized, for example:

-#define RETURN_IF_ERROR_IN_SCOPE(C, CALL){\+#define RETURN_IF_ERROR_IN_SCOPE(C, CALL) do{\ if ((CALL) < 0){\ compiler_exit_scope((C)); \ return ERROR; \ } \ -}+} while (0)

On other hand, this is an opportunity to vertically align line continuation characters as recommended by PEP 7. I am not sure that this is a good reason to do this, it is better to ask anybody who currently regularly works with this code.

cc @iritkatriel, @JelleZijlstra?

@iritkatriel
Copy link
Member

The diff could be minimized, for example:

-#define RETURN_IF_ERROR_IN_SCOPE(C, CALL){\+#define RETURN_IF_ERROR_IN_SCOPE(C, CALL) do{\ if ((CALL) < 0){\ compiler_exit_scope((C)); \ return ERROR; \ } \ -}+} while (0)

On other hand, this is an opportunity to vertically align line continuation characters as recommended by PEP 7. I am not sure that this is a good reason to do this, it is better to ask anybody who currently regularly works with this code.

cc @iritkatriel, @JelleZijlstra?

I don't have a preference. Happy with either way.

@picnixz
Copy link
MemberAuthor

The diff could be minimized, for example:

Actually, in another PR, I was asked to indent it. And other constructions seem to indent the do and the while as well. I also think it's a bit closer to how we write functions in the rest of the code, namely with { on a separate line.

I tried to be consistent and only align the line continuations if it's new code (note that most of the time, line continuations were already aligned so, I just continued with the existing style, lucky me).

@picnixzpicnixz changed the title gh-120017: use 'do-while(0)' in some compile.c multi-line macrosgh-120017: use 'do-while(0)' in some codegen.c multi-line macrosNov 7, 2024
@picnixzpicnixz changed the title gh-120017: use 'do-while(0)' in some codegen.c multi-line macrosgh-120017: use 'do-while(0)' in some {codegen,compile}.c multi-line macrosNov 7, 2024
@iritkatrieliritkatriel merged commit c222441 into python:mainNov 7, 2024
@picnixzpicnixz deleted the use-do-while-in-compile branch November 7, 2024 23:51
picnixz added a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@picnixz@iritkatriel@serhiy-storchaka@sobolevn@nineteendo