Fix for Issue #1258 #1298

allanleal · 2018-02-26T09:26:48Z

list_caster::load method will now check for a Python string and prevent its automatic conversion to a list.
This should fix the issue "pybind11/stl.h converts string to vector #1258" (#1258)

jagerman · 2018-02-28T02:45:42Z

Looks fine, but can you add a simple test case for this to the PR (i.e. something that goes to the wrong overload when passed a string). test_methods_and_attributes.cpp/.py seems like a good place.

allanleal · 2018-04-17T10:03:29Z

Could any of you guys help me on making these automated tests pass? This is a just minor change in pybind11 to fix issue #1258.

ax3l · 2018-09-02T11:14:18Z

@allanleal can you rebase this PR? I appreciate if this is fixed as well for one of my projects :)

ax3l · 2018-09-04T09:51:37Z

tests/test_stl.py

 def test_stl_ownership():
    cstats = ConstructorStats.get(m.Placeholder)
    assert cstats.alive() == 0
    r = m.test_stl_ownership()
    assert len(r) == 1
    del r
    assert cstats.alive() == 0
+


unnecessary white space change, please remove :)

ax3l · 2018-09-04T09:53:48Z

@allanleal please really rebase this branch, ~~I think the debug errors you see were recently fixed in #1438~~

allanleal · 2018-09-04T11:29:47Z

@ax3l Could you please take the lead on this? This is a very simple bug fix, yet, for some reason, it does not go though during the CI phase, and I'm not able to fix this right now. Thanks.

ax3l · 2018-09-04T12:03:11Z

@allanleal I can try :) Can you add me as a collaborator with write access to https://github.com/allanleal/pybind11?

I am not a pybind11 maintainer and can due to that not push to your branch.

allanleal · 2018-09-04T12:06:18Z

Thanks, @ax3l - just added you.

allanleal · 2018-09-04T12:14:59Z

Current status is: "Awaiting ax3l's response". I was given no option - just the name of the user. Maybe you need to accept it first.

…

On 04.09.2018 14:12, Axel Huebl wrote: @allanleal <https://github.com/allanleal> hm, still can't write? Did you add "write" access? Otherwise, just create a team that has write access to your repo and add me there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1298 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFjkNN3fw4a3xJ_bacfmmVvL25Oq_Grbks5uXm4pgaJpZM4SS6P5>.

ax3l · 2018-09-04T18:40:37Z

I can locally verify the failing test in debug mode. Also, there is a violation of the one definition rule in type_caster that needs a fix.

ax3l · 2018-09-04T19:39:25Z

tests/test_stl.cpp

@@ -237,6 +237,10 @@ TEST_SUBMODULE(stl, m) {
    // test_stl_pass_by_pointer
    m.def("stl_pass_by_pointer", [](std::vector<int>* v) { return *v; }, "v"_a=nullptr);

+    // #1258: pybind11/stl.h converts string to vector<string>
+    m.def("func_with_string_or_vector_string_arg_overload", [](std::vector<std::string>) { return 0; });


this line triggers a ODR warning:

include/pybind11/stl.h:179:49: warning: type ‘struct type_caster’ violates the C++ One Definition Rule [-Wodr] template <typename Type, typename Alloc> struct type_caster<std::vector<Type, Alloc>> ^ tests/test_opaque_types.cpp:19:1: note: a type with the same name but different base type is defined in another translation unit PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>); ^ include/pybind11/stl.h:137:49: note: type name ‘pybind11::detail::list_caster<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >’ should match type name ‘pybind11::detail::type_caster_base<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >’ template <typename Type, typename Value> struct list_caster { ^ include/pybind11/cast.h:814:32: note: the incompatible type is defined here template <typename type> class type_caster_base : public type_caster_generic { ^ tests/test_opaque_types.cpp:19:1: note: type ‘struct type_caster’ itself violate the C++ One Definition Rule PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>); ^ include/pybind11/stl.h:179:49: note: the incompatible type is defined here template <typename Type, typename Alloc> struct type_caster<std::vector<Type, Alloc>>

probably a

PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>);

missing

hm, now both py arrays and py lists are std::list< std::string >( ) ...

ax3l · 2018-09-04T20:59:22Z

@allanleal shouldn't the same treatment go to the array_caster (std::vector) and not only to list_caster (std::list)?

allanleal · 2018-10-02T11:41:24Z

Hi @ax3l , did it fail again?

@jagerman Could you please help here? This is a very simple fix, but not passing the tests.

ax3l · 2018-10-02T13:39:56Z

@allanleal yep, sorry I don't know what's off here

jagerman · 2018-10-02T15:06:56Z

There is an ODR violation happening (you can see the warning in the gcc-7 build) because the PYBIND11_MAKE_OPAQUE added in test_stl.cpp is in the wrong place — it's inside the #elif defined(PYBIND11_TEST_BOOST) && ..., moving to after the #endif ought to resolve at least that issue. With that fix it builds for me here under gcc-8 without warnings and passes the tests.

ax3l · 2018-10-02T15:15:10Z

Thanks a lot, I did not see that. Will fix it and push again.

list_caster::load method will now check for a Python string and prevent its automatic conversion to a list. This should fix the issue "pybind11/stl.h converts string to vector<string> #1258" (#1258)

ax3l · 2018-10-02T16:02:07Z

Yay, that fixed it! :)

allanleal · 2018-10-03T08:28:34Z

Great - thanks @ax3l. @jagerman can we finally see this merged? Thank you so much for revising this.

jagerman · 2018-10-04T00:46:16Z

This looks good to me. @wjakob ?

jagerman · 2018-10-04T00:47:30Z

Oh, one last request -- can you add a changelog entry for it to docs/changelog.rst?

ax3l · 2018-10-04T07:35:48Z

@jagerman shall I start a 2.2.5 section or add it to 2.3.0?

jagerman · 2018-10-04T12:22:23Z

2.3.0; it seems like more than just a minor .x bug fix.

ax3l · 2018-10-04T14:15:18Z

@jagerman alright, added the changelog and CI is green :)

allanleal · 2018-10-11T05:52:45Z

@ax3l, thanks a lot.

@jagerman, just a kind remind if we could see this merged soon. Is there anything else missing?

wjakob · 2018-10-11T08:28:05Z

Looks good, thanks!

allanleal · 2018-10-11T08:29:28Z

Great, thanks!

ax3l reviewed Sep 4, 2018

View reviewed changes

allanleal added 2 commits October 2, 2018 17:20

Fix for Issue #1258

a7a9de1

list_caster::load method will now check for a Python string and prevent its automatic conversion to a list. This should fix the issue "pybind11/stl.h converts string to vector<string> #1258" (#1258)

Added tests for fix of issue #1258

7642ea1

Changelog: stl string auto-conversion

ab14c68

allanleal mentioned this pull request Oct 11, 2018

Add tests reaktoro/reaktoro#9

Merged

wjakob merged commit e76dff7 into pybind:master Oct 11, 2018

ax3l deleted the patch-1 branch October 11, 2018 11:27

allanleal mentioned this pull request Dec 19, 2018

Pybind11 is now (temporarily) located in thirdparty dir reaktoro/reaktoro#40

Merged

2 tasks

rwgk mentioned this pull request Dec 1, 2020

Fixes #1807: 2.3.0 regression: <class 'bytes'> -> std::vector<uint8_t> #2198

Open

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#1298

Closed

Fix for Issue #1258 #1298

Fix for Issue #1258 #1298

Uh oh!

Conversation

allanleal commented Feb 26, 2018

Uh oh!

jagerman commented Feb 28, 2018

Uh oh!

allanleal commented Apr 17, 2018

Uh oh!

ax3l commented Sep 2, 2018

Uh oh!

ax3l Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanleal commented Sep 4, 2018

Uh oh!

ax3l commented Sep 4, 2018

Uh oh!

allanleal commented Sep 4, 2018

Uh oh!

allanleal commented Sep 4, 2018 via email

Uh oh!

ax3l commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ax3l Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 4, 2018

Uh oh!

allanleal commented Oct 2, 2018

Uh oh!

ax3l commented Oct 2, 2018

Uh oh!

jagerman commented Oct 2, 2018

Uh oh!

ax3l commented Oct 2, 2018

Uh oh!

ax3l commented Oct 2, 2018

Uh oh!

allanleal commented Oct 3, 2018

Uh oh!

jagerman commented Oct 4, 2018

Uh oh!

jagerman commented Oct 4, 2018

Uh oh!

ax3l commented Oct 4, 2018

Uh oh!

jagerman commented Oct 4, 2018

Uh oh!

ax3l commented Oct 4, 2018

Uh oh!

allanleal commented Oct 11, 2018

Uh oh!

wjakob commented Oct 11, 2018

Uh oh!

allanleal commented Oct 11, 2018 via email

Uh oh!

Uh oh!

ax3l commented Sep 4, 2018 •

edited

Loading

ax3l commented Sep 4, 2018 •

edited

Loading