Repository navigation
gh-107603: Argument Clinic can emit includes - #108486
vstinner merged 3 commits into
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| core_includes: bool = False | ||
| core_includes: bool = False, | ||
| # needed if core_includes is true | ||
| clinic: Clinic | None = None, |
There was a problem hiding this comment.
Should we just pass clinic.includes here, rather than the full clinic object?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Opened #108581 as a sketch.
A
Sorry, something went wrong.
| line = f'#include "{include}"' | ||
| line = line.ljust(35) + f'// {reason}\n' |
There was a problem hiding this comment.
Should we add a comment to explain the indent amount?
| 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.
There was a problem hiding this comment.
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.
| if name in self.includes: | ||
| # Mention a single reason is enough, no need to list all of them |
There was a problem hiding this comment.
For generated code we could list all the known reasons?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
* 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.
Always require clinic.
|
I rebased my PR on top of the merge AC change adding support for the limited C API. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Remove _PyLong_UnsignedShort_Converter() from the public C API: move the function to the internal pycore_long.h header.