Skip to content

gh-157649: Fix Py_CLEAR()/Py_SETREF() on C++ - #158070

Open
vstinner wants to merge 1 commit into
python:mainfrom
vstinner:py_clear_auto
Open

vstinner wants to merge 1 commit into
python:mainfrom
vstinner:py_clear_auto

Conversation

@vstinner

@vstinner vstinner commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

On C++, do not use decltype() in _Py_TYPEOF since it produces invalid code in Py_CLEAR() and Py_SETREF(). Instead, implement Py_CLEAR() and Py_SETREF() using "auto" on C++11 and newer.

Add Py_CLEAR() and Py_SETREF() tests on an array.

On C++, do not use decltype() in _Py_TYPEOF since it produces invalid
code in Py_CLEAR() and Py_SETREF(). Instead, implement Py_CLEAR() and
Py_SETREF() using "auto" on C++11 and newer.

Add Py_CLEAR() and Py_SETREF() tests on an array.
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit d448889 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F158070%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 24, 2026
@vstinner

Copy link
Copy Markdown
Member Author

@h-vetinari

Copy link
Copy Markdown
Contributor

This LGTM, though I'm not very knowledgeable about the macros here.

To contribute something slightly non-trivial in response to your ping, the history of why decltype isn't suitable, resp. that there's no unified infra for this between C & C++, is actually quite fun/tragic. To the best of my understanding (based on public posts, I'm not privy to committee-internal details):

  • Bjarne Stroustrup originally wanted to add typeof to C++ as early as 2002 (see references below)
  • C++11 added decltype, though the rationale is more fully explained in the earlier papers, e.g. N1607 (the last in the series to contain a discussion on typeof)
  • Part of the problem was that C++ - in contrast to C - needed to deal with reference-ness, and whether to preserve or keep it. N1478 etc. argue that consequently, two variants of typeof would be necessary, which they didn't want to do. I can't find a direct quote right now (though P2958 below comes close), but I believe there was also an aspect of not taking typeof away from the C standard, and waiting for WG14 to come up with something.
  • [many years later] typeof finally got added to C23, and its variant for removing all qualifier even got renamed specifically for compatibility with compatibility of C++'s original intent and use in mind
  • typeof & typeof_unqual were then reproposed for C++, and tragically/hilariously died a violent committee death.
  • compiler authors provide relief with __typeof__ that does the right thing everywhere, the end.

Looking at the diff, this provides an idea perhaps, in that the C++ case could be ordered after #ifdef _Py_TYPEOF, which would relegate the auto-based implementation only to those compilers who don't know __typeof__; this would provide a more uniform implementation for the 99% case IMO.

Comment thread Include/cpython/object.h
* Py_DECREF().
*/
#ifdef _Py_TYPEOF
#define Py_XSETREF(dst, src) \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd adjust Py_XSETREF like Py_SETREF and add a test using it. Currently, it would fail because MSVC does not support __typeof__ in C++, see below.

Comment thread Include/pyport.h
#elif defined(__GNUC__) || defined(__clang__) || \
(defined(_MSC_VER) && _MSC_VER >= 1939)
#elif (defined(__GNUC__) || defined(__clang__) \
|| (defined(_MSC_VER) && _MSC_VER >= 1939))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
|| (defined(_MSC_VER) && _MSC_VER >= 1939))
|| (defined(_MSC_VER) && _MSC_VER >= 1939 && !defined(__cplusplus)))

MSVC does not support __typeof__ in C++, so I'd guard with !defined(__cplusplus). 1

Footnotes

  1. because __STDC_VERSION__ is only set if option /std is used, https://godbolt.org/z/78cP4crsW. ↩

@bedevere-app

bedevere-app Bot commented Sep 24, 2026

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

@chris-eibl

Copy link
Copy Markdown
Member

__typeof__; this would provide a more uniform implementation for the 99% case IMO.

True for gcc and clang but not MSVC. Also, strictly speaking, __typeof__ is not part of standard C++.
Your reorder suggestion would be a more defensive change, because this is how it was used in 3.15, where MSVC always took the memcpy path (except when in C23 mode).

@h-vetinari

h-vetinari commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

True for gcc and clang but not MSVC.

MSVC would work itself out via your suggested && !defined(__cplusplus). Then the order would be:

  • #ifdef _Py_TYPEOF (roughly equivalent to "has __typeof__") --> use that
  • #elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L) --> use C++11 auto
  • #else --> use memcpy

I'd prefer this because it keeps C++ out of the default path.

@chris-eibl

chris-eibl commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Yeah, as said above, that would be the least change in behaviour (and no change wrt gcc and clang) compared to 3.15.

Which means auto would not be used in all C++11 code paths (only for MSVC).

Whether we'd like to unify for C++11 or favor __typeof__ is a decision I am on the fence - IMHO there is no clear win here.

@da-woods

Copy link
Copy Markdown
Contributor

I've run it on the relevant parts of the Cython CI and it fixes what I was hoping that it would fix.

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.

5 participants