GeezerVison

🤖 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.

cc @da-woods @h-vetinari @chris-eibl @picnixz

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.

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!

__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).

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.

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.

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

Ok, I changed the implementation to use _Py_TYPEOF if available, and only use C++ auto if _Py_TYPEOF is not available. memcpy() fallback is used in the two other implementations are not available (MSVC in C++03 mode?).

Adding tests to test_cext and test_cppext became more and more annoying over time, since these "basic C/C++ extensions" became more complex over time. I merged test_cppext into test_cext (PR #158176) to share most code (init, setup, extension.c), especially extension.c which is now used by C and C++. By the way, I also made a change to check for reference leaks (and I had to fix a test for that!): PR #158227.

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

Oh ok, fixed. Since _Py_TYPEOF() was not used by default on C++ on my first implementation, this bug was not catched by test_cppext.

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.

Ooops, I forgot that Py_XSETREF() as a copy-pasta implementation copied from Py_SETREF(). I added tests and also updated Py_XSETREF() implementation to use C++ auto.

@chris-eibl: Oh, the PR is still red because you requested changes. I addressed your review. Would you mind reviewing the updated PR?

Once GitHub Action CI will pass, I will schedule a buildbot run to run test_cext on buildbots.

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

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.

Thanks Victor, I think this is the best we can get with the lowest risk: the gcc / clang basically stays untouched, and for MSVC we should get the typeof branch for C and auto for C++.

memcpy() fallback is used in the two other implementations are not available (MSVC in C++03 mode?).

I think we'll rarely get there, except when not gcc, clang or MSVC.

In case of MSVC for versions >= VS 2017, C++14 is the minimum, there is no /std switch for anything lower than C++14.

Hence I think

You should currently see a warning for test_build_cpp03 in case of MSVC saying "unsupported option /std:c++03" and basically just doing the same as test_build. Which I think does the same as test_build_cpp14, because /std:c++14 is the default?

If test_build_cpp03 is changed, I would prefer to do in a separated PR.

Tests passed a wide diversity of buildbots, I enable auto-merge.

test_dtrace fails on s390x, but that's a known and unrelated issue.

Thanks for reviews! I merged my change.

Py_SETREF() macros family is a gift which keeps giving: see issue #161185 about strict aliasing.