[For discussion purpose] - Used unique_ptr instead of auto_ptr when c++11 is used. #29

Bertram25 · 2015-08-23T22:05:29Z

(CC @endoalir for info.)

After some fiddling and some research, I manage to obtain a luabind version capable to compile without any warnings when using c++11, and that seems to run Valyria Tear rather fine, according to my latest tests... You can see the changes I made in this PR.

YET, (because nice things never comes free), it seems those changes aren't perfect according to the following issue: HoverRace/HoverRace#260

It seems the following adapted commit is also needed: halmd-org@1853787 because otherwise the following issue may happen: HoverRace/HoverRace#259

However, it seems HoverRace is using the main luabind repository and that other bugs may come into game, though. So I made this PR mainly to know what to do next in order to get a c++11-warning-free-and-functional luabind PR. Do you think my PR is enough, or do you have hints anywhere at how I should handle the issue around boost::shared_ptr vs std::shared_ptr?

Thanks a lot for your repository in any case and for your help, and best regards,

fhoefling · 2015-08-25T08:55:30Z

Am 24.08.2015, 00:05 Uhr, schrieb Yohann Ferreira
[email protected]:

Hey @Oberon00 :D

(CC @endoalir for info.)

After some fiddling and some research, I manage to obtain a luabind
version capable to compile without any warnings when using c++11, and
that seems to run Valyria Tear rather fine, according to my latest
tests... You can see the changes I made in this PR.

YET, (because nice things never comes free), it seems those changes
aren't perfect according to the following issue:
HoverRace/HoverRace#260

It seems the following adapted commit is also needed:
halmd-org@1853787
because otherwise the following issue may happen:
HoverRace/HoverRace#259

In HALMD, we've been using luabind (=luaponte) with C++11 since long
without any issues. Our version is perhaps a bit out-dated since I didn't
have time to follow the recent developments in luabind, but it brings all
the functionality we need (as of luabind 0.9.1) and works with the most
recent Boost releases.

I'd love to revert from luaponte to luabind and to use a library which is
supported by a (small) community. Still I'm not sure which luabind branch
to start with, build-improvements seemed to be the most useful one for me
but it hasn't been continued. I think the most urgent need is a kind of
release schedule and finally a new release of luabind (following the old
0.9.1). Featurewise, the various master branches (from different people)
are in a good shape as far as I can tell. However, just adding commits to
"master" is not enough, it does not tell how stable the current tip of the
branch is. And stability is one of the most important criteria to make a
large project depend on a certain library.

BTW, there is at least one failing test concerning "out_value" vs.
"pure_out_value", see commit a1199b5.

Regards,

Felix

Bertram25 · 2015-08-27T08:41:10Z

Hey @fhoefling :)

In HALMD, we've been using luabind (=luaponte) with C++11 since long without any issues. Our version is perhaps a bit out-dated since I didn't have time to follow the recent developments in luabind, but it brings all the functionality we need (as of luabind 0.9.1) and works with the most recent Boost releases.

It's good to know. The only thing to consider though is that it seems required to keep the former C++ norm supported (or keep std::auto_ptr and don't use nullptr) optionally.
Maybe @Oberon00 will be able to tell more about this.

I'd love to revert from luaponte to luabind and to use a library which is supported by a (small) community. Still I'm not sure which luabind branch to start with, build-improvements seemed to be the most useful one for me but it hasn't been continued. I think the most urgent need is a kind of release schedule and finally a new release of luabind (following the old 0.9.1).

Here you're speaking about the mother luabind, which many are considering a dead upstream.
The small community you're speaking about is IMHO either around @rpavlik or @Oberon00 's forks.
So, I wonder: Do you have access to luabind to start and make it live again, for instance? Also, see below.

Featurewise, the various master branches (from different people) are in a good shape as far as I can tell. However, just adding commits to "master" is not enough, it does not tell how stable the current tip of the branch is. And stability is one of the most important criteria to make a large project depend on a certain library.

IMHO, @Oberon00 has done a good job in trying to keep the lib clean and import mostly bug fixes and taking care of not doing anything too experimental. So, would luabind's authors be away for good, I would rather focus on bringing improvements here. (As I'm trying to do with this current PR).

In your opinion, what features would be lacking here before you would consider adoption, for instance?
Could we have a common effort on a alive fork and IMO achieve what everyone wants to achieve: A good common new reference release, even from a fork? (After all, it's the way Open Source work sometimes.)

BTW, there is at least one failing test concerning "out_value" vs. "pure_out_value", see commit a1199b5.

k, created issue: #30

Oberon00 · 2015-08-27T17:24:26Z

Since C++17 will probably remove auto_ptr entirely and MSVC 14 (2015) already allows users to disable it, I consider this pull request a quite reasonable idea.

However, your approach of using #ifdef everywhere is suboptimal: Better use a typedef ??? luabind::detail::unique_ptr + a luabind::detail::move function and define that conditionally in one place (probably a new detail header).

Also do not make this configurable in the CMake file (or if an issue arises that forces the offering of a user configurable "override switch" this should be done via a #cmakedefine in build_information.hpp) but instead rely on Boost.Config's BOOST_NO_CXX11_SMART_PTR. Because older versions of Boost that did not have this macro should also be supported, an additional check for LUABIND_NO_RVALUE_REFERENCES would be good too. See config.hpp and std_shared_ptr_converter.hpp for examples.

When that is done, the -Wno-deprecated-declarations flag can be removed from the CMakeLists.txt.

[…] do you have hints anywhere at how I should handle the issue around boost::shared_ptr vs std::shared_ptr?

Do you mean unique_ptr? Or what issue around shared_ptr do you mean?

Bertram25 · 2015-10-21T21:59:43Z

@Oberon00 I'm overwhelmed by other tasks but I'll come back here to upgrade PR eventually. Thanks for your patience! :)

akien-mga · 2016-04-13T16:46:28Z

Hey guys, what's the status of this PR?

Oberon00 · 2016-04-13T17:12:01Z

I won't merge the pull request until the many #ifdefs are replaced with a typedef & wrapper move function, as I commented above. Also, because it seems that this pull request is just a cosmetic improvement, I don't plan to allocate time for doing so myself though.

Bertram25 · 2016-04-24T22:22:39Z

I won't merge the pull request until the many #ifdefs are replaced with a typedef & wrapper move function, as I commented above

Yeah, I've got some job left on this one. I didn't forget about it, but there is quite some things on my plate atm, sorry. :)

Also, because it seems that this pull request is just a cosmetic improvement, I don't plan to allocate time for doing so myself though.

Here, I disagree, removing deprecation warnings is no cosmetic thing, especially when the c++14 norm will be the standard.

Oberon00 · 2016-04-25T16:55:29Z

Here, I disagree, removing deprecation warnings is no cosmetic thing, especially when the c++14 norm will be the standard.

Good point, I haven't thought about that. luabind is compiled with -Wno-deprecated, but you are right that forcing user code to also disable this (generally useful) warning is a problem.

…hen using c++11.

Move instead of copy smart pointers, using std::move. Add overload to get_pointer() to retrieve raw pointer. Based on Peter Colberg's patch: halmd-org@19d9a14

Oberon00 · 2020-05-24T12:53:18Z

CMakeLists.txt

@@ -105,6 +105,10 @@ if(LUABIND_NO_STD_SHARED_PTR)
    add_definitions(-DLUABIND_NO_STD_SHARED_PTR)
 endif()

+if(LUABIND_USE_CXX11)


Can you instead use LUABIND_NO_STD_UNIQUE_PTR to mirror LUABIND_NO_STD_SHARED_PTR? Ideally, this should also try to use BOOST_NO_CXX11_SMART_PTR and a CMake option should only be necessary as a fallback.

Also, I think this should be in build_information.hpp instead of being added via add_definitions, but it's been too long ago, so I'm not completely sure.

Yes, this needs to be in build_information.hpp. This PR is somehow live in Ubuntu 20.04 LTS though the ValyriaTear/luabind repo. It broke our ryzom/ryzomcore build, since luabind is built with the option, but the headers get included without LUABIND_USE_CXX11 (unless we hardcode the option in our own repo).

The alternative is of course auto-detecting, and not giving the option to users.

Oberon00 · 2020-05-24T12:55:28Z

luabind/adopt_policy.hpp

@@ -96,7 +96,11 @@ namespace luabind { namespace detail
    template <class T>
    struct pointer_or_default<void, T>
    {
+#ifdef LUABIND_USE_CXX11


You have this #ifdef in multiple places. I'd prefer placing it in a central header, like luabind_memory.hpp or similar with a single namespace luabind { typedef unique_auto_ptr ...; }.

Oberon00 · 2020-05-24T12:59:57Z

luabind/detail/constructor.hpp

@@ -54,7 +57,11 @@ struct construct_aux<0, T, Pointer, Signature>
        void* storage = self->allocate(sizeof(holder_type));

        self->set_instance(new (storage) holder_type(
+#ifdef LUABIND_USE_CXX11
+            std::move(ptr), registered_class<T>::id, naked_ptr));


Similarly to the centralized typedef a centralized ifdefed LUABIND_MOVE macro would be useful. Maybe it could also be a luabind::move function, but that might be overkill.

Oberon00 · 2020-05-24T13:01:09Z

luabind/scope.hpp

        scope(scope const& other_);
        ~scope();

        scope& operator=(scope const& other_);

-        scope& operator,(scope s);
+        scope& operator,(const scope& s);


Reading the implementation in scope.cpp of this, I'm concerned. Was a simple def("foo", &foo), def("bar", &bar) previously causing a double-delete?

Would scope& without const work here, similar to https://en.cppreference.com/w/cpp/memory/auto_ptr/auto_ptr ?

I naively tried scope& and the unit tests blew up on me. It looks like scope is trying to implement its own homebrew unique_ptr mechanism and that is all tied up in using const scope& to trigger the complemented copy constructors to orchestrate the proper ownership transfer.

It felt like fixing this for c++03 would require some serious plumbing work. However, under c++11, implementing the proper move semantics seems to address the major use cases.

Oberon00 · 2020-05-24T13:12:26Z

Hi @Bertram25! It seems this PR comes from your master branch? Do you still intend to merge this? If so, please see the comments above. I think you address some important things here, so I would be glad 😃

kchang718 · 2021-08-19T06:54:22Z

Hi guys, we've been looking at these patches and have applied @Bertram25 's changes along with @Oberon00 's comments to support unique_ptr. These changes are on PR #47. @jameslee5656 and I have looked over these changes to our best abilities and they run ok in our codebase.

We've also did some work to address the operator,() issues in scope.hpp for c++11.

Please take a look when time allows.

Bertram25 mentioned this pull request Aug 23, 2015

Luabind + Memleak fix discussion ValyriaTear/ValyriaTear#480

Closed

2 tasks

Bertram25 mentioned this pull request Aug 27, 2015

Failing unit test: "out_value" vs. "pure_out_value" #30

Open

Oberon00 mentioned this pull request Oct 21, 2015

Replace deprecated auto_ptr with unique_ptr #32

Open

Bertram25 force-pushed the master branch from 5267b65 to 9f05359 Compare February 22, 2016 17:50

Bertram25 force-pushed the master branch from 9f05359 to d082834 Compare February 29, 2016 21:25

Bertram25 force-pushed the master branch from d082834 to 76d4061 Compare November 8, 2017 10:55

Yohann Ferreira added 2 commits May 24, 2020 13:10

Made luabind use unique_ptr as a functional replacement to auto_ptr w…

c6cf32b

…hen using c++11.

C++11: replace deprecated std::auto_ptr with std::unique_ptr

a9287d3

Move instead of copy smart pointers, using std::move. Add overload to get_pointer() to retrieve raw pointer. Based on Peter Colberg's patch: halmd-org@19d9a14

Bertram25 force-pushed the master branch from 76d4061 to a9287d3 Compare May 24, 2020 11:13

Oberon00 reviewed May 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[For discussion purpose] - Used unique_ptr instead of auto_ptr when c++11 is used. #29

[For discussion purpose] - Used unique_ptr instead of auto_ptr when c++11 is used. #29

Bertram25 commented Aug 23, 2015

fhoefling commented Aug 25, 2015

Bertram25 commented Aug 27, 2015

Oberon00 commented Aug 27, 2015

Bertram25 commented Oct 21, 2015

akien-mga commented Apr 13, 2016

Oberon00 commented Apr 13, 2016

Bertram25 commented Apr 24, 2016 •

edited

Loading

Oberon00 commented Apr 25, 2016

Oberon00 May 24, 2020

kaetemi Sep 28, 2021 •

edited

Loading

kaetemi Sep 28, 2021

Oberon00 May 24, 2020

Oberon00 May 24, 2020

Oberon00 May 24, 2020

kchang718 Aug 19, 2021

Oberon00 commented May 24, 2020

kchang718 commented Aug 19, 2021

[For discussion purpose] - Used unique_ptr instead of auto_ptr when c++11 is used. #29

Are you sure you want to change the base?

[For discussion purpose] - Used unique_ptr instead of auto_ptr when c++11 is used. #29

Conversation

Bertram25 commented Aug 23, 2015

fhoefling commented Aug 25, 2015

Bertram25 commented Aug 27, 2015

Oberon00 commented Aug 27, 2015

Bertram25 commented Oct 21, 2015

akien-mga commented Apr 13, 2016

Oberon00 commented Apr 13, 2016

Bertram25 commented Apr 24, 2016 • edited Loading

Oberon00 commented Apr 25, 2016

Oberon00 May 24, 2020

Choose a reason for hiding this comment

kaetemi Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

kaetemi Sep 28, 2021

Choose a reason for hiding this comment

Oberon00 May 24, 2020

Choose a reason for hiding this comment

Oberon00 May 24, 2020

Choose a reason for hiding this comment

Oberon00 May 24, 2020

Choose a reason for hiding this comment

kchang718 Aug 19, 2021

Choose a reason for hiding this comment

Oberon00 commented May 24, 2020

kchang718 commented Aug 19, 2021

Bertram25 commented Apr 24, 2016 •

edited

Loading

kaetemi Sep 28, 2021 •

edited

Loading