Skip to content

gh-107603: Argument Clinic: Only include pycore_gc.h if needed - #108726

Merged
vstinner merged 5 commits into
python:mainfrom
vstinner:clinic_includes3
Aug 31, 2023
Merged

gh-107603: Argument Clinic: Only include pycore_gc.h if needed#108726
vstinner merged 5 commits into
python:mainfrom
vstinner:clinic_includes3

Conversation

@vstinner

@vstinnervstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member

Argument Clinic now only includes pycore_gc.h if PyGC_Head is needed, and only includes pycore_runtime.h if _Py_ID() is needed.

  • Add 'condition' optional argument to Clinic.add_include().
  • deprecate_keyword_use() includes pycore_runtime.h when using the _PyID() function.
  • Fix rendering of includes: comments start at the column 35.
  • Mark PC/clinic/_wmimodule.cpp.h and "Objects/stringlib/clinic/*.h.h" header files as generated in .gitattributes.

Effects:

  • 42 header files generated by AC no longer include the internal C API, instead of 4 header files before. For example, Modules/clinic/_abc.c.h no longer includes the internal C API.
  • Fix _testclinic_depr.c.h: it now always includes pycore_runtime.h to get _Py_ID().

Argument Clinic now only includes pycore_gc.h if PyGC_Head is needed,
and only includes pycore_runtime.h if _Py_ID() is needed.
* Add 'condition' optional argument to Clinic.add_include().
* deprecate_keyword_use() includes pycore_runtime.h when using
the _PyID() function.
* Fix rendering of includes: comments start at the column 35.
* Mark PC/clinic/_wmimodule.cpp.h and
"Objects/stringlib/clinic/*.h.h" header files as generated in
.gitattributes.
Effects:
* 42 header files generated by AC no longer include the internal C
API, instead of 4 header files before. For example,
Modules/clinic/_abc.c.h no longer includes the internal C API.
* Fix _testclinic_depr.c.h: it now always includes pycore_runtime.h
to get _Py_ID().
Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py
Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

@AlexWaygood: Please review my updated PR. I addressed your latest review.

@erlend-aaslanderlend-aasland changed the title gh-107603: AC only includes pycore_gc.h if neededgh-107603: Argument Clinic: Only include pycore_gc.h if neededAug 31, 2023

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also don't like to pass around clinic like that, but if Alex is fine with it, it's ok.

It is nice to avoid unneeded includes, so thanks for that clean-up.

@vstinner

Copy link
Copy Markdown
MemberAuthor

I also don't like to pass around clinic like that, but if Alex is fine with it, it's ok.

In general, limited_capi bool is now passed as an argument. clinic is only passed as an argument when clinic.add_include() should be called.

@AlexWaygoodAlexWaygood 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

I also don't like to pass around clinic like that

Yeah, agreed that this maybe isn't the best pattern, but we can always refactor at a later date — I think this is fine for now.

Thanks for the updates @vstinner!

@vstinner
vstinner merged commit ad73674 into python:mainAug 31, 2023
@vstinner
vstinner deleted the clinic_includes3 branch August 31, 2023 21:42
@vstinner

Copy link
Copy Markdown
MemberAuthor

Merged, thanks for reviews @AlexWaygood and @erlend-aasland!

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.

4 participants

@vstinner@erlend-aasland@AlexWaygood@bedevere-bot