gh-112205: Support @getter annotation from AC#112396

corona10 · 2023-11-25T11:14:05Z

Issue: Support getters and setters in Argument Clinic #112205

corona10 · 2023-11-25T11:15:35Z

This is the first draft version of getter annotation for AC.
Please leave some feedback for changes, and then I will add test code at the end.

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <[email protected]>

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <[email protected]>

corona10 · 2023-11-25T13:48:38Z

Now logic for clinic.py become straightforward. PTAL :)

corona10 · 2023-11-25T14:34:08Z

And I am going to implementer @setter annotation with separate PR :)

Tools/clinic/clinic.py

corona10 · 2023-11-25T14:47:29Z

Tools/clinic/clinic.py

    """)
+    GETTERDEF_PROTOTYPE_DEFINE: Final[str] = normalize_snippet(r"""
+        #define {methoddef_name}    \
+            {{"{name}", {methoddef_cast}{c_basename}{methoddef_cast_end}, NULL, NULL}},


We may need to reform the template for the setter case too. But not sure we should handle this at this PR :)

erlend-aasland · 2023-11-27T11:14:46Z

Thanks, Donghee! Can you also create a docs PR for the devguide?

corona10 · 2023-11-27T11:21:11Z

Thanks, Donghee! Can you also create a docs PR for the devguide?

Sure, Do you have any recommended section for this?

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <[email protected]>

corona10 · 2023-11-27T11:47:32Z

@erlend-aasland @AlexWaygood
Here is the documentation PR: python/devguide#1232
For @setter, I will add it once the directive is implemented.

Tools/clinic/clinic.py

colesbury

This looks great. I don't have any comments on top of what Erlend and Alex wrote.

AlexWaygood

The Python code LGTM, thanks! As always, I'll leave review of the generated C code to the other two :)

corona10 · 2023-11-28T01:44:48Z

I will wait @erlend-aasland 1-2days :)

erlend-aasland · 2023-11-28T08:46:05Z

Tools/clinic/clinic.py

            parser_code: list[str] | None
-            if not requires_defining_class:
+            if f.kind is GETTER:
+                flags = "NULL"


This looks strange to me; AFAICS, we don't use flags for getters (methoddef_flags is not included in the template). And if we were to supply a zero default for flags, it would have to be an int literal (0, not NULL).

Ideally we should not have to specify flags here, but a lot of the following code expect it to be defined. I suggest we either set it to the empty string or the zero int literal.

Suggested change

flags = "NULL"

flags = "0"

Suggested change

flags = "NULL"

flags = "" # This should end up unused

@AlexWaygood?

Ah please ignore my previous comment, now I understood what you want to say :)

You may already know: the reason I don't use methodef_flags to the template is due to the structure of getset.

cpython/Include/descrobject.h

Lines 11 to 17 in a194938

struct PyGetSetDef {

const char *name;

getter get;

setter set;

const char *doc;

void *closure;

};

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.

We need to refactor some of the code while implementing @setter, Can we handle this issue on the @setter PR? or Do you want to handle it as this PR?

dde933e
Remove unnecessary template :)

I'm not sure my suggested comment is good enough :) Wording it more explicit might be better (cc. @AlexWaygood).

I think this is fine... or, at least, I can't think of anything better :)

Looking at this function gives me a headache; this change doesn't really make my head hurt significantly more tbh :)

Yes, the urge to refactor is growing. I suggest we take a stab at it pretty soon, Alex :)

Tools/clinic/clinic.py

corona10 · 2023-11-29T23:59:39Z

@erlend-aasland If the current status is enough to be merged, I will merge this PR at this weekend :)
cc @AlexWaygood @colesbury

pythongh-112205: Support @Getter annotation from AC

72d9018

corona10 added the topic-free-threading label Nov 25, 2023

corona10 requested a review from colesbury November 25, 2023 11:14

corona10 requested review from AlexWaygood and erlend-aasland as code owners November 25, 2023 11:14

bedevere-app bot added the awaiting core review label Nov 25, 2023

bedevere-app bot mentioned this pull request Nov 25, 2023

Support getters and setters in Argument Clinic #112205

Closed

corona10 added the skip news label Nov 25, 2023

Update define macro

ea83973

AlexWaygood reviewed Nov 25, 2023

View reviewed changes

Tools/clinic/clinic.py Outdated Show resolved Hide resolved

Tools/clinic/clinic.py Outdated Show resolved Hide resolved

corona10 and others added 2 commits November 25, 2023 20:58

Apply suggestions from code review

969adf9

Co-authored-by: Alex Waygood <[email protected]>

nit

716b8d9

corona10 requested a review from AlexWaygood November 25, 2023 12:00

corona10 added 2 commits November 25, 2023 22:00

Add test case

c5fe2b2

Add test case

2d9800d

AlexWaygood reviewed Nov 25, 2023

View reviewed changes

Tools/clinic/clinic.py Outdated Show resolved Hide resolved

corona10 and others added 5 commits November 25, 2023 22:29

Clean up

442c575

Update Tools/clinic/clinic.py

e0fb271

Co-authored-by: Alex Waygood <[email protected]>

Update logic

0778eb7

Update

d5b1751

Clean up

7f4bfa8

corona10 requested a review from AlexWaygood November 25, 2023 14:00

corona10 commented Nov 25, 2023

View reviewed changes

Tools/clinic/clinic.py Show resolved Hide resolved

corona10 commented Nov 25, 2023

View reviewed changes

AlexWaygood reviewed Nov 27, 2023

View reviewed changes

Tools/clinic/clinic.py Outdated Show resolved Hide resolved

Update Tools/clinic/clinic.py

fe46de0

Co-authored-by: Alex Waygood <[email protected]>

AlexWaygood reviewed Nov 27, 2023

View reviewed changes

Tools/clinic/clinic.py Show resolved Hide resolved

Address code review

0892d0d

colesbury reviewed Nov 27, 2023

View reviewed changes

AlexWaygood approved these changes Nov 27, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Nov 27, 2023

erlend-aasland reviewed Nov 28, 2023

View reviewed changes

Address code review

769f6cd

corona10 requested a review from erlend-aasland November 28, 2023 13:39

corona10 added 2 commits November 28, 2023 23:07

Address code review

dde933e

Change template key

d381b5e

erlend-aasland reviewed Nov 28, 2023

View reviewed changes

Tools/clinic/clinic.py Outdated Show resolved Hide resolved

corona10 added 2 commits November 29, 2023 06:51

Use Py_UNUSED for context

351ba08

Update test

f53a035

erlend-aasland approved these changes Nov 30, 2023

View reviewed changes

corona10 merged commit 7eeea13 into python:main Nov 30, 2023

bedevere-app bot removed the awaiting merge label Nov 30, 2023

corona10 deleted the gh-112205 branch November 30, 2023 10:40

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-112205: Support @Getter annotation from AC (pythongh-112396)

fae9ef6

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-112205: Support @Getter annotation from AC (pythongh-112396)

9bbf3ee

	struct PyGetSetDef {
	const char *name;
	getter get;
	setter set;
	const char *doc;
	void *closure;
	};

Uh oh!

Conversation

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

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Nov 27, 2023

Uh oh!

corona10 commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 commented Nov 27, 2023

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

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!

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!

corona10 commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corona10 commented Nov 25, 2023 •

edited by bedevere-app bot

Loading

corona10 commented Nov 25, 2023 •

edited

Loading

corona10 commented Nov 25, 2023 •

edited

Loading

corona10 commented Nov 27, 2023 •

edited

Loading

corona10 commented Nov 29, 2023 •

edited

Loading