Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function pybind11_select_caster uses ADL to select the caster. #3643

Closed
wants to merge 7 commits into from

Conversation

amauryfa
Copy link

Description

Instead of the type_caster template specialization, it is possible to register the conversions with a function declaration, which will be found with ADL.

namespace std {
string_caster pybind11_select_caster(std::string*);
}

The parameter is a pointer to the C++ (intrinsic) type. The returned type is the caster class. This function should have no implementation!

  • There is no ODR violation when different translation units define different conversions: distinct modules might choose different behavior. This is the main motivation of this change.
  • The declarations need to be defined in the original namespace of the converted C++ type.
  • The default behavior did not change: type_caster<T> will be used as a fallback.
  • Because type_caster<T> not always used, I had to update some places with make_caster<T>.

So far I do not propose to convert the existing casters. I updated the one for std::string just to prove the concept, and check that it works on all compilers.

- There is no ODR violation when different translation units define
  different conversions.
- The declarations need to be defined in the original namespace.
@Skylion007 Skylion007 requested a review from rwgk January 25, 2022 16:34
@Skylion007
Copy link
Collaborator

@henryiii Do we finally need to update MSYS2?

@Skylion007 Skylion007 requested a review from henryiii January 25, 2022 16:36
@rwgk
Copy link
Collaborator

rwgk commented Jan 25, 2022

The 2 mingw failures are some infrastructure issue that we started seeing yesterday, unrelated to this PR.

@laramiel
Copy link
Contributor

There's an ICE when building with MSVC 2015. Now that it's 2022, I suspect that it's time to retire that compiler.

d:\a\pybind11\pybind11\include\pybind11\cast.h(35): fatal error C1001: An internal error has occurred in the compiler. [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
 (compiler file 'msc1.cpp', line 1468)
  To work around this problem, try simplifying or changing the program near the locations listed above.

This ICE was reported to MS--it appears to be a MSVC 2015 update 3 specific bug--but it was long enough ago that the link has become dead:
https://connect.microsoft.com/VisualStudio/feedback/details/2869042

It appears similar to a decltype() adjacent bug reported to boost in 2018:
https://svn.boost.org/trac10/ticket/13393#ticket

@henryiii
Copy link
Collaborator

We are going to drop Python 2.7-3.5 soon, that was when I was planning to retire the compiler. With windows-2016 being removed in March, March is also a good time to retire it. But I'm honestly okay to retire it sooner.

What about the failures on 2017?

@rwgk
Copy link
Collaborator

rwgk commented Jan 27, 2022

But I'm honestly okay to retire it sooner.

Fantastic!

  • That gives me a strong argument to prioritize the Python 2.7 & 3.5 removal over other things.

  • It will also get this oddity off my back:

    // Working around MSVC 2015 bug. const-correctness is lost.

  • And I can be more relaxed when our toddler decides to bang on the keyboard of the only laptop I have for MSVC 2015 testing.

(It's late here, sorry for rambling.)

@laramiel
Copy link
Contributor

laramiel commented Jan 27, 2022

@henryiii I haven't yet looked too closely at those, so hard to say yet. I may look tomorrow, but the environment setup is somewhat challenging.

Edit: It looks like it's trying to build in C++14 mode to me, but the description line isn't exactly clear about that, and there's no log that I immediately see with the full command line.

Run cmake -S . -B build -G "Visual Studio 15 2017" -A x64 -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=14 
  cmake -S . -B build -G "Visual Studio 15 2017" -A x64 -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=14 

And the error is in:

          props = m.OptionalRefSensitiveProperties()
          assert int(props.access_by_ref) == 42
  >       assert int(props.access_by_copy) == 42
  E       assert -572662307 == 42
  E        +  where -572662307 = int(<EnumType.???: -572662307>)
  E        +    where <EnumType.???: -572662307> = <pybind11_tests.stl.OptionalRefSensitiveProperties object at 0x000001812334FAF0>.access_by_copy

Where OptionalRefSensitiveProperties uses template <typename T> class ReferenceSensitiveOptional {} which has a comment that references issue #3330

@rwgk
Copy link
Collaborator

rwgk commented Jan 27, 2022

The error in @laramiel 's previous comment came up before:

#3376 (comment)

If you look there, the next comment has a fix (smart_holder): fc5d70d

I already forgot all the details, I hope this is a useful clue.

@henryiii
Copy link
Collaborator

henryiii commented Jan 31, 2022

MSVC 2015 2017 only really supports C++14. They basically implemented 11 and 14 in one shot, because they were behind.

I wonder if fixing the 2017 error will help 2015?

@henryiii
Copy link
Collaborator

henryiii commented Jan 31, 2022

To actually merge this, we probably should either revert the std::string change, or add similar changes for the other casters, they should be consistent. If there's a chance it will work on MSVC 2015, maybe we could see if reverting the std::string change (hopefully dropping in something similar in as a test) causes this to pass? If the only problem is related to using this, I'd not be that worried about 2015. (I think this is not why it's breaking, though).

@rwgk
Copy link
Collaborator

rwgk commented Jan 31, 2022

If there's a chance it will work on MSVC 2015, maybe we could see if reverting the std::string

Yeah, I was thinking the same, and maybe @amauryfa, too?
IIUC those changes were mainly to have a quick way to exercise the new feature.
I'm thinking undoing those changes and adding dedicated new tests is best.

@rwgk
Copy link
Collaborator

rwgk commented Apr 11, 2022

Work continued under #3862.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants