Reference N3035 20.9.10.2.5 para 5. Ensure all specializations are fixed. This is what is there now: void reset(pointer __p = pointer()) { if (__p != get()) { get_deleter()(get()); std::get<0>(_M_t) = __p; } } This is what it should be changed to: void reset(pointer __p = pointer()) { pointer old_p = get(); if (__p != old_p) { std::get<0>(_M_t) = __p; get_deleter()(old_p); } }
To be clear: we are probably going to implement this, and more, in time for 4.5.0, but we can't play this game: having a PR for each unimplemented item in the last WP would immediately overflow Bugzilla. In general, Bugzilla is for *bugs* in normal releases, not for tracking the development of the experimental implementation of the ongoing drafts of the new standard.
This didn't change in N3035, N3000 had the same wording.
Chris, can you please double check this?
I see that you are around... ;)
OK, I'm back and have had time to look at this. I vaguely remember noticing that the assignment and the deleter invocation happened in the wrong order in our implementation, but I must have forgotten about it as I don't have any uncommitted change (or even a comment) in my working copy. The suggested change is still incorrect: the wrong condition is checked. The deleter must be invoked when old_p != NULL, rather than when old_p != p. Consider: unique_ptr<int> p1; p1.reset(new int); The deleter should not be invoked by the call to reset, because old_p == nullptr. Another case: unique_ptr<int> p(new int); p.reset(p.get()); p.release(); This should not leak, but with the suggested change it will, because the deleter will not get invoked. A better implementation would be (untested): void reset(pointer __p = pointer()) { pointer __old = get(); std::get<0>(_M_t) = __p; if (__old != 0) std::get<1>(_M_t)(__old); }
Indeed, thanks Jon. Shall we implement this for 4.5.0, or we had better wait for nullptr / nullptr_t, what do you think?
I think it should be fixed for 4.5 and then updated when nullptr is available. I assume that LWG 834 will be accepted in some form, so we will need an update at some point anyway, to use nullptr in release and operator bool and in the assertions in operator* and operator->
Actually, we could just use pointer() everywhere, which would work today and would be equivalent to using nullptr, assuming the current proposed resolution of 834 or something similar. I would be very surprised if 834 is resolved in a way that allows different semantics for get() == nullptr and get() == pointer() i.e. explicit operator bool() const { return get() == pointer() ? false : true; } // Modifiers. pointer release() { pointer __p = get(); std::get<0>(_M_t) = pointer(); return __p; }
Agreed. In two days or so I can take care of committing these changes.
Subject: Re: std::unique_ptr::reset() does not conform to N3035. I see your point. I think it should still check for resetting to the same value to avoid duplicate deletes later. terry void reset(pointer __p = pointer()) { pointer old_p = get(); if (__p != old_p) { std::get<0>(_M_t) = __p; if (old_p != pointer()) // <-------- added this line get_deleter()(old_p); } } ----- Original Message ----- From: "jwakely dot gcc at gmail dot com" <gcc-bugzilla@gcc.gnu.org> To: <tjgolubi@netins.net> Sent: Monday, March 01, 2010 9:05 AM Subject: [Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035. > > > ------- Comment #5 from jwakely dot gcc at gmail dot com 2010-03-01 > 15:05 ------- > OK, I'm back and have had time to look at this. I vaguely remember > noticing > that the assignment and the deleter invocation happened in the wrong order > in > our implementation, but I must have forgotten about it as I don't have any > uncommitted change (or even a comment) in my working copy. > > The suggested change is still incorrect: the wrong condition is checked. > The > deleter must be invoked when old_p != NULL, rather than when old_p != p. > Consider: > > unique_ptr<int> p1; > p1.reset(new int); > > The deleter should not be invoked by the call to reset, because old_p == > nullptr. > > Another case: > > unique_ptr<int> p(new int); > p.reset(p.get()); > p.release(); > > This should not leak, but with the suggested change it will, because the > deleter will not get invoked. > > A better implementation would be (untested): > > void > reset(pointer __p = pointer()) > { > pointer __old = get(); > std::get<0>(_M_t) = __p; > if (__old != 0) > std::get<1>(_M_t)(__old); > } > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183 > > ------- You are receiving this mail because: ------- > You reported the bug, or are watching the reporter.
(In reply to comment #10) > I think it should still check for resetting to the same value to avoid > duplicate deletes later. I disagree, double delete can only happen in the case of a programming error. Look at my second example in comment #5 - it may not be a very good idea to write code like that, but it is technically correct (the stored pointer is deleted when reset() is called, then released so it won't be deleted again) and should not leak the stored pointer. If you check for the same value then you will not call the deleter during reset and will leak. I am testing a fix now, but I will not include your suggestion to check p!=old, the C++0x draft requires a certain behaviour and I will implement that.
Bear in mind that a custom deleter with custom pointer type might have very different semantics for comparing pointer values and for invoking the deleter. Consider a custom D::pointer which keeps a generation count, which is not used when comparing for equality: template<class T> struct MyDeleter { struct pointer { int generation; T* ptr; pointer() : generation(), ptr() { } bool operator==(pointer rhs) { return ptr == rhs.ptr; } }; void operator()(pointer p) const { do_something(ptr.generation); delete p.ptr; } void do_something(int gen); }; Your suggested implementation would not update the value, because the equality test is true, but that would be wrong i.e. you would have failed to reset the unique_ptr as requested by the user.
Subject: Re: std::unique_ptr::reset() does not conform to N3035. I think you are correct now. Thank you for the explanation. terry ----- Original Message ----- From: "redi at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> To: <tjgolubi@netins.net> Sent: Monday, March 01, 2010 4:38 PM Subject: [Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035. > > > ------- Comment #12 from redi at gcc dot gnu dot org 2010-03-01 > 22:38 ------- > Bear in mind that a custom deleter with custom pointer type might have > very > different semantics for comparing pointer values and for invoking the > deleter. > Consider a custom D::pointer which keeps a generation count, which is not > used > when comparing for equality: > > template<class T> > struct MyDeleter { > struct pointer { > int generation; > T* ptr; > pointer() : generation(), ptr() { } > bool operator==(pointer rhs) > { return ptr == rhs.ptr; } > }; > void operator()(pointer p) const > { > do_something(ptr.generation); > delete p.ptr; > } > void do_something(int gen); > }; > > Your suggested implementation would not update the value, because the > equality > test is true, but that would be wrong i.e. you would have failed to reset > the > unique_ptr as requested by the user. > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183 > > ------- You are receiving this mail because: ------- > You reported the bug, or are watching the reporter.
Subject: Bug 43183 Author: redi Date: Tue Mar 2 00:40:28 2010 New Revision: 157158 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=157158 Log: 2010-03-02 Jonathan Wakely <jwakely.gcc@gmail.com> PR libstdc++/43183 * include/bits/unique_ptr.h (reset): Fix as per working paper. (operator*, operator->, operator[], operator bool, release): Use pointer's null value instead of 0. * testsuite/20_util/unique_ptr/assign/assign_neg.cc: Adjust. * testsuite/20_util/unique_ptr/modifiers/reset_neg.cc: Adjust. * testsuite/20_util/unique_ptr/modifiers/43183.cc: New. Added: trunk/libstdc++-v3/testsuite/20_util/unique_ptr/modifiers/43183.cc Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/unique_ptr.h trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/assign_neg.cc trunk/libstdc++-v3/testsuite/20_util/unique_ptr/modifiers/reset_neg.cc
Fixed. Version checked in uses swap: void reset(pointer __p = pointer()) { using std::swap; swap(std::get<0>(_M_t), __p); if (__p != pointer()) get_deleter()(__p); }
I might have caused a regression with this change: FAIL: 30_threads/promise/members/set_value3.cc execution test WARNING: program timed out. Will investigate later today...
The 30_threads/promise/members/set_value3.cc test had a latent bug which was revealed by the unique_ptr fix. I'll change the test.