Skip to content

[3.12] gh-130740: Move some stdbool.h includes after Python.h (#130738)#130757

Merged
picnixz merged 3 commits into
python:3.12from
picnixz:bp/312/214562ed4ddc248b007f718ed92ebcc0c3669611-130740
Mar 4, 2025
Merged

[3.12] gh-130740: Move some stdbool.h includes after Python.h (#130738)#130757
picnixz merged 3 commits into
python:3.12from
picnixz:bp/312/214562ed4ddc248b007f718ed92ebcc0c3669611-130740

Conversation

@picnixz

@picnixz picnixz commented Mar 2, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

chouquette and others added 2 commits March 2, 2025 11:05
@chouquette @picnixz
pythongh-130740: Move some stdbool.h includes after Python.h (pyt…
6486103 
…hon#130738)

Move some `#include <stdbool.h>` after `#include "Python.h"` when `pyconfig.h` is not
included first and when we are in a platform-agnostic context. This is to avoid having
features defined by `stdbool.h` before those decided by `Python.h`.
@picnixz
backport
6a3dfb7
This was referenced Mar 2, 2025
Merged
Closed
@picnixz picnixz marked this pull request as ready for review March 2, 2025 10:38
@picnixz

picnixz commented Mar 3, 2025

Copy link
Copy Markdown
Member Author

For this one, I'd like approval from @markshannon as I modified the generator cases.

vstinner
vstinner reviewed Mar 3, 2025
View reviewed changes
Comment thread Tools/cases_generator/generate_cases.py
self.out.write_raw(f"// Do not edit!\n")

self.out.write_raw("\n")
self.out.write_raw("#include <stdbool.h>")

@vstinner vstinner Mar 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you add <stdbool.h> include?

@picnixz picnixz Mar 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because there are some bool inside the generate_metadata.h file which, without this include, doesn't compile. Previously, it was because we included stdbool.h before including this file so it was impliticitly available (see https://github.com/python/cpython/actions/runs/13614386994/job/38055588576?pr=130757)

@markshannon

markshannon commented Mar 3, 2025

Copy link
Copy Markdown
Member

The change to Tools/cases_generator/generate_cases.py seems fine. Harmless at worst.

@markshannon

markshannon commented Mar 3, 2025

Copy link
Copy Markdown
Member

Don't the generated files need to be regenerated?

@picnixz

picnixz commented Mar 3, 2025

Copy link
Copy Markdown
Member Author

Don't the generated files need to be regenerated?

I should have regenerated them? efd75da#diff-f862bc60824529a25ba378be850ecc6762f03c6c944266d30ac7172d10132498. Or are there more rules that I needed to run?

@vstinner

vstinner commented Mar 3, 2025

Copy link
Copy Markdown
Member

I downloaded the PR and ran make regen-all: there is no change.

vstinner
vstinner approved these changes Mar 3, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@picnixz

picnixz commented Mar 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

make regen-all

I think some of the targets need to be regenerated separately (this may include the regen-cases but I don't remember well) The other files that could have been regenerated are generated_cases.c.h but I'm not changing their output as they don't have a "bool" (or at least my compiler didn't complain about it). What really mattered was opcode_metadata.h.

EDIT: regen-all was sufficient, my bad!

@picnixz picnixz self-assigned this Mar 3, 2025
@picnixz picnixz merged commit 7ce5f15 into python:3.12 Mar 4, 2025
@picnixz picnixz deleted the bp/312/214562ed4ddc248b007f718ed92ebcc0c3669611-130740 branch March 4, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Assignees

@picnixz picnixz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@picnixz @markshannon @vstinner @chouquette