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 <Alex.Waygood@Gmail.com>

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

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

@@ -865,6 +869,10 @@ class CLanguage(Language):
        #define {methoddef_name}    \
            {{"{name}", {methoddef_cast}{c_basename}{methoddef_cast_end}, {methoddef_flags}, {c_basename}__doc__}},
    """)
+    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 <Alex.Waygood@Gmail.com>

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

@@ -1217,7 +1228,11 @@ def parser_body(
        parsearg: str | None
        if not parameters:
            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 erlend-aasland and AlexWaygood 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 <Alex.Waygood@Gmail.com>

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 <Alex.Waygood@Gmail.com>

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 <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Nov 27, 2023

View reviewed changes

Tools/clinic/clinic.py Outdated 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
34 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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 25, 2023

corona10 Nov 25, 2023

erlend-aasland commented Nov 27, 2023

corona10 commented Nov 27, 2023 •

edited

Loading

corona10 commented Nov 27, 2023

colesbury left a comment

AlexWaygood left a comment

corona10 commented Nov 28, 2023

erlend-aasland Nov 28, 2023

This comment was marked as resolved.

corona10 Nov 28, 2023

corona10 Nov 28, 2023

erlend-aasland Nov 28, 2023

corona10 Nov 28, 2023

corona10 Nov 28, 2023

erlend-aasland Nov 28, 2023

AlexWaygood Nov 29, 2023

erlend-aasland Nov 30, 2023

corona10 commented Nov 29, 2023 •

edited

Loading

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

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

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

Conversation

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 25, 2023

Choose a reason for hiding this comment

erlend-aasland commented Nov 27, 2023

corona10 commented Nov 27, 2023 • edited Loading

corona10 commented Nov 27, 2023

colesbury left a comment

Choose a reason for hiding this comment

AlexWaygood left a comment

Choose a reason for hiding this comment

corona10 commented Nov 28, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Nov 29, 2023 • edited Loading

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