GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-107603: Argument Clinic can emit includes by vstinner · Pull Request #108486 · python/cpython

Conversation

vstinner commented Aug 25, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Remove _PyLong_UnsignedShort_Converter() from the public C API: move the function to the internal pycore_long.h header.

Add Clinic.add_include() method
Add CConverter.include and CConverter.add_include()
Printer.print_block() gets a second parameter: clinic.

Issue: Argument Clinic includes internal headers in generated output unconditionally #107603

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner requested review from AlexWaygood and erlend-aasland as code owners

August 25, 2023 15:35

bedevere-bot mentioned this pull request

Aug 25, 2023

Argument Clinic includes internal headers in generated output unconditionally #107603

Closed

bedevere-bot added the awaiting core review label

Aug 25, 2023

vstinner commented Aug 25, 2023

Copy link

Copy Markdown

Member Author

cc @erlend-aasland @AlexWaygood

I included _PyLong_UnsignedShort_Converter() removal in this PR to show how the feature can be used, but once reviewed, I can consider to extract this change into a separated PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added the skip news label

Aug 25, 2023

vstinner force-pushed the clinic_includes branch from 4e138c5 to cdadfa1 Compare

August 25, 2023 20:50

AA-Turner reviewed

Aug 25, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/clinic/clinic.py Outdated

		core_includes: bool = False
		core_includes: bool = False,
		# needed if core_includes is true
		clinic: Clinic \| None = None,

AA-Turner Aug 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we just pass clinic.includes here, rather than the full clinic object?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Aug 25, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I prefer to pass the Clinic class, it may useful later to get more attributes. But I don't understand well the AC design. IMO the whole idea of passing Clinic here is broken, it should not be the role of a "BlockPrinter" to generate these #include :-( Well, I prefer to move step by step, since AC is complicated and (for me) hard to modify.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood Aug 28, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmm, I agree with @AA-Turner here: I'd prefer if we only passed exactly what we need, rather than passing the whole clinic object. I think it would make the code more explicit, and make the function easier to understand just by looking at its signature

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

AA-Turner Aug 28, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Opened #108581 as a sketch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/clinic/clinic.py

Comment on lines +2162 to +2178

		line = f'#include "{include}"'
		line = line.ljust(35) + f'// {reason}\n'

AA-Turner Aug 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we add a comment to explain the indent amount?

Suggested change

	line = f'#include "{include}"'
	line = line.ljust(35) + f'// {reason}\n'
	include = f'"{include}"'
	line = f'#include {include: <25} // {reason}\n'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Aug 25, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

35 is my arbitrary convention that I'm using in Python. I prefer to keep ljust(35) since it reminds me the 35th column :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/clinic/clinic.py

Comment on lines +2436 to +2454

		if name in self.includes:
		# Mention a single reason is enough, no need to list all of them

AA-Turner Aug 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

For generated code we could list all the known reasons?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Aug 25, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't see the point. It would generate giant lines for little value.

For #include, I like to show at least one imported symbol to explain why the include was needed. In manually written code, I use it to remove unused imports time to time. IMO here it's interesting to repeat this habit to explain why an include is needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner added 3 commits

August 25, 2023 23:26


          pythongh-107603: Argument Clinic can emit includes

830fb64

* Add Clinic.add_include() method
* Add CConverter.include and CConverter.add_include()
* Printer.print_block() gets a second parameter: clinic.
* Remove duplicated declaration of "clinic" global variable.


          Address AA-Turner's coding style review

68d499e


          Simplify print_block()

e07d07c

Always require clinic.

vstinner force-pushed the clinic_includes branch from 900a002 to e07d07c Compare

August 25, 2023 21:28

vstinner commented Aug 25, 2023

Copy link

Copy Markdown

Member Author

I rebased my PR on top of the merge AC change adding support for the limited C API.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner merged commit 73d33c1 into python:main

Aug 25, 2023

bedevere-bot removed the awaiting core review label

Aug 25, 2023

vstinner deleted the clinic_includes branch

August 25, 2023 22:01

vstinner commented Aug 25, 2023

Copy link

Copy Markdown

Member Author

As a follow-up, I wrote PR #108499 which removes private _PyLong converters from the public C API: move them to the internal C API. The change adds #include "pycore_include.h" to the few clinic files using it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood mentioned this pull request

Aug 28, 2023

gh-108494: Argument Clinic: fix option group for Limited C API #108574

Merged

AA-Turner mentioned this pull request

Aug 28, 2023

GH-107603: Clinic: Pass specific attributes to print_block() #108581

Merged

Reviewers

AA-Turner

AA-Turner left review comments

erlend-aasland

Awaiting requested review from erlend-aasland

erlend-aasland is a code owner

AlexWaygood

Awaiting requested review from AlexWaygood

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

vstinner commented Aug 25, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Aug 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Aug 25, 2023

Uh oh!

vstinner commented Aug 25, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Aug 25, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.