Skip to content

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commented Aug 31, 2023

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().
@vstinner
Copy link
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
Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

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
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.

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

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!

@vstinnervstinner merged commit ad73674 into python:mainAug 31, 2023
@vstinnervstinner deleted the clinic_includes3 branch August 31, 2023 21:42
@vstinner
Copy link
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