Skip to content

gh-127341: Argument Clinic: fix compiler warnings for getters with docstrings - #127310

Merged
erlend-aasland merged 5 commits into
python:mainfrom
ZeroIntensity:fix-clinic-getsetdef-docstrings
Nov 29, 2024
Merged

erlend-aasland merged 5 commits into
python:mainfrom
ZeroIntensity:fix-clinic-getsetdef-docstrings

Conversation

@ZeroIntensity

@ZeroIntensity ZeroIntensity commented Nov 26, 2024

Copy link
Copy Markdown
Member

Finally got enough connection to submit a PR 😄

Co-authored-by: Erlend E. Aasland erlend.aasland@protonmail.com

@skirpichev

Copy link
Copy Markdown
Member

It seems you forgot make clinic, or it was intentional, to reduce patch size? Change looks fine.

@ZeroIntensity

Copy link
Copy Markdown
Member Author

It seems you forgot make clinic, or it was intentional, to reduce patch size? Change looks fine.

Yeah, that would be it. Fixed!

@skirpichev
skirpichev self-requested a review November 27, 2024 02:24
@erlend-aasland

Copy link
Copy Markdown
Contributor

I don't like the original approach that was taken when the GETSET_ clinic templates were added. How about we simplify it to this:

diff --git a/Tools/clinic/libclinic/parse_args.py b/Tools/clinic/libclinic/parse_args.py
index fc2d9fe9870..a57d729bec5 100644
--- a/Tools/clinic/libclinic/parse_args.py
+++ b/Tools/clinic/libclinic/parse_args.py
@@ -146,7 +146,7 @@ def declare_parser(
 GETSET_DOCSTRING_PROTOTYPE_STRVAR: Final[str] = libclinic.normalize_snippet("""
     PyDoc_STRVAR({getset_basename}__doc__,
     {docstring});
-    #define {getset_basename}_HAS_DOCSTR
+    #define {getset_basename}_DOCSTR {getset_basename}__doc__
 """)
 IMPL_DEFINITION_PROTOTYPE: Final[str] = libclinic.normalize_snippet("""
     static {impl_return_type}
@@ -157,9 +157,7 @@ def declare_parser(
         {{"{name}", {methoddef_cast}{c_basename}{methoddef_cast_end}, {methoddef_flags}, {c_basename}__doc__}},
 """)
 GETTERDEF_PROTOTYPE_DEFINE: Final[str] = libclinic.normalize_snippet(r"""
-    #if defined({getset_basename}_HAS_DOCSTR)
-    #  define {getset_basename}_DOCSTR {getset_basename}__doc__
-    #else
+    #if !defined({getset_basename}_DOCSTR)
     #  define {getset_basename}_DOCSTR NULL
     #endif
     #if defined({getset_name}_GETSETDEF)
@@ -170,9 +168,7 @@ def declare_parser(
     #endif
 """)
 SETTERDEF_PROTOTYPE_DEFINE: Final[str] = libclinic.normalize_snippet(r"""
-    #if defined({getset_name}_HAS_DOCSTR)
-    #  define {getset_basename}_DOCSTR {getset_basename}__doc__
-    #else
+    #if !defined({getset_basename}_DOCSTR)
     #  define {getset_basename}_DOCSTR NULL
     #endif
     #if defined({getset_name}_GETSETDEF)

@erlend-aasland

Copy link
Copy Markdown
Contributor

BTW, I think we should record why this was a bug in an issue, and then link this PR to that issue.

@ZeroIntensity ZeroIntensity changed the title Clinic: Fix compiler warnings when using a docstring with a getter gh-127341: Clinic: Fix compiler warnings when using a docstring with a getter Nov 27, 2024
@erlend-aasland erlend-aasland changed the title gh-127341: Clinic: Fix compiler warnings when using a docstring with a getter gh-127341: Argument Clinic: fix compiler warnings for getters with docstrings Nov 29, 2024

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for identifying and fixing this!

@erlend-aasland
erlend-aasland merged commit 9949091 into python:main Nov 29, 2024
@erlend-aasland erlend-aasland added the needs backport to 3.13 bugs and security fixes label Nov 29, 2024
@miss-islington-app

This comment was marked as outdated.

@miss-islington-app

Copy link
Copy Markdown

Sorry, @ZeroIntensity and @erlend-aasland, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 99490913a08adcf2fe5e69b82772a829ec462275 3.13

@ZeroIntensity

Copy link
Copy Markdown
Member Author

I'll deal with backporting.

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request Nov 29, 2024
…tters with docstrings (pythonGH-127310)

(cherry picked from commit 9949091)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Erlend E. Aasland <erlend@python.org>
@bedevere-app

bedevere-app Bot commented Nov 29, 2024

Copy link
Copy Markdown

GH-127431 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Nov 29, 2024
erlend-aasland added a commit that referenced this pull request Nov 30, 2024
…with docstrings (GH-127310) (#127431)

(cherry picked from commit 9949091)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@ZeroIntensity
ZeroIntensity deleted the fix-clinic-getsetdef-docstrings branch December 14, 2024 16:41
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…ith docstrings (python#127310)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
…ith docstrings (python#127310)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clinic causes compiler warnings when using a getter and setter with a docstring

3 participants