Skip to content

DEAD END: Make make_caster unique_to_translation_unit #3931

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

Closed
wants to merge 18 commits into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented May 8, 2022

Description

DEAD END

Small experiment (3239d48) on top of PR #3862.

DEAD END

The approach below does not solve the ODR problem. The essence is captured here:

The power_less function violates the ODR because it drops the U template parameter (roughly corresponds to pybind11 cpp_function::initialize).

The more_power_less function violates the ODR transitively (roughly corresponds to cpp_function::dispatcher).

See also: dead-end experiment changing pybind11 (on top of master):

Original PR description below:


Objective: ISO C++ standard-compliant support for translation-unit-specific caster classes.

Informed by observations from an isolated experiment under https://github.com/rwgk/stuff/tree/master/caster_odr:

  • engine.h — model for pybind11/cast.h
  • car.cc — model for a pybind11-based Python extension module
  • truck.cc — model for another pybind11-based Python extension
  • vehicle_specs.cc (main()) — model for an application that imports both extensions
  • clang++ -std=c++11 car.cc truck.cc vehicle_specs.cc

The example output below exhibits

  • the observable consequences of ODR-non-conformance in the 1st & 2nd ./a.out outputs (car and truck powers are mixed up),
  • and that using unique_to_translation_unit fixes the observable consequences (3rd & 4th ./a.out outputs).

The caster_odr results were reproduced with:

  • Debian clang 13 (gLinux; below)
  • Apple clang 12 (macOS 12)
  • MSVC 2015 (Windows 11)

Important:

  • The caster_odr experiment informed the experiment under this PR, but is different! The pybind11_select_caster mechanism it not part of the caster_odr experiment.
  • Even without using unique_to_translation_unit, test_select_caster + test_select_caster_alt (included in PR Add pybind11_select_caster #3862) does not exhibit observable consequences of ODR-non-conformance (https://eel.is/c++draft/temp.dep.candidate#1).

Questions (two ways of looking at the same question, really):

  • Does 3239d48 make test_select_caster + test_select_caster_alt ISO C++ standard-compliant?
  • Can someone suggest an experiment that exhibits observable consequences of the ODR-non-conformance for make_caster as under PR Add pybind11_select_caster #3862 (i.e. without unique_to_translation_unit)?
rwgk.c.googlers.com:~/clone/stuff/caster_odr $ ./compile_and_run.sh
++ clang++ -std=c++11 car.cc truck.cc vehicle_specs.cc
++ ./a.out
short car   power:   100
long  car   power:   200
short truck power:   100
long  truck power:   200
++ clang++ -std=c++11 truck.cc car.cc vehicle_specs.cc
++ ./a.out
short car   power:   300
long  car   power:   400
short truck power:   300
long  truck power:   400
++ clang++ -std=c++11 -DUSE_UNIQUE_TO_TRANSLATION_UNIT car.cc truck.cc vehicle_specs.cc
++ ./a.out
short car   power:   100
long  car   power:   200
short truck power:   300
long  truck power:   400
++ clang++ -std=c++11 -DUSE_UNIQUE_TO_TRANSLATION_UNIT truck.cc car.cc vehicle_specs.cc
++ ./a.out
short car   power:   100
long  car   power:   200
short truck power:   300
long  truck power:   400
++ rm a.out

Suggested changelog entry:

rwgk added a commit to rwgk/stuff that referenced this pull request May 9, 2022
Models pybind11_select_caster (pybind/pybind11#3931) instead of the older type_caster specialization mechanics.
@rwgk rwgk changed the title Make make_caster unique_to_translation_unit DEAD END: Make make_caster unique_to_translation_unit May 10, 2022
@rwgk rwgk closed this May 10, 2022
rwgk added a commit to laramiel/pybind11 that referenced this pull request May 13, 2022
For full background see: pybind#3931

TL;DR: The work under PR pybind#3931 made it obvious that test_select_caster_alt was too simple for deriving meaningful conclusions.
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.

2 participants