Uh oh!
There was an error while loading. Please reload this page.
gh-107603: Argument Clinic can emit includes - #108486
Conversation
vstinner
commented
Aug 25, 2023
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. |
Uh oh!
There was an error while loading. Please reload this page.
| 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?
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.
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
Uh oh!
There was an error while loading. Please reload this page.
| 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' |
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 :-)
| 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?
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* 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.
vstinner
commented
Aug 25, 2023
I rebased my PR on top of the merge AC change adding support for the limited C API. |
vstinner
commented
Aug 25, 2023
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 |
Remove _PyLong_UnsignedShort_Converter() from the public C API: move the function to the internal pycore_long.h header.