[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #16 from jwakely dot gcc at gmail dot com 2010-03-02 11:41 --- 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... -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #17 from jwakely dot gcc at gmail dot com 2010-03-02 12:44 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[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_ptrint p1;
p1.reset(new int);
The deleter should not be invoked by the call to reset, because old_p ==
nullptr.
Another case:
unique_ptrint 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::get0(_M_t) = __p;
if (__old != 0)
std::get1(_M_t)(__old);
}
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #6 from paolo dot carlini at oracle dot com 2010-03-01 16:08 --- Indeed, thanks Jon. Shall we implement this for 4.5.0, or we had better wait for nullptr / nullptr_t, what do you think? -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #7 from jwakely dot gcc at gmail dot com 2010-03-01 16:23 --- 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- -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #8 from jwakely dot gcc at gmail dot com 2010-03-01 16:35
---
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::get0(_M_t) = pointer();
return __p;
}
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #9 from paolo dot carlini at oracle dot com 2010-03-01 17:06 --- Agreed. In two days or so I can take care of committing these changes. -- paolo dot carlini at oracle dot com changed: What|Removed |Added Status|UNCONFIRMED |NEW Ever Confirmed|0 |1 Last reconfirmed|-00-00 00:00:00 |2010-03-01 17:06:21 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
-- redi at gcc dot gnu dot org changed: What|Removed |Added AssignedTo|unassigned at gcc dot gnu |redi at gcc dot gnu dot org |dot org | Status|NEW |ASSIGNED Last reconfirmed|2010-03-01 17:06:21 |2010-03-01 20:56:36 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #10 from tjgolubi at netins dot net 2010-03-01 22:22 ---
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::get0(_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-bugzi...@gcc.gnu.org
To: tjgol...@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_ptrint p1;
p1.reset(new int);
The deleter should not be invoked by the call to reset, because old_p ==
nullptr.
Another case:
unique_ptrint 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::get0(_M_t) = __p;
if (__old != 0)
std::get1(_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.
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #11 from redi at gcc dot gnu dot org 2010-03-01 22:30 --- (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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[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:
templateclass 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
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #13 from tjgolubi at netins dot net 2010-03-01 23:23 ---
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-bugzi...@gcc.gnu.org
To: tjgol...@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:
templateclass 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.
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #14 from redi at gcc dot gnu dot org 2010-03-02 00:40 --- Subject: Bug 43183 Author: redi Date: Tue Mar 2 00:40:28 2010 New Revision: 157158 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=157158 Log: 2010-03-02 Jonathan Wakely jwakely@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 -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #15 from redi at gcc dot gnu dot org 2010-03-02 00:41 ---
Fixed. Version checked in uses swap:
void
reset(pointer __p = pointer())
{
using std::swap;
swap(std::get0(_M_t), __p);
if (__p != pointer())
get_deleter()(__p);
}
--
redi at gcc dot gnu dot org changed:
What|Removed |Added
Status|ASSIGNED|RESOLVED
Resolution||FIXED
Target Milestone|--- |4.5.0
Version|unknown |4.5.0
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #4 from paolo dot carlini at oracle dot com 2010-02-26 18:04 --- I see that you are around... ;) -- paolo dot carlini at oracle dot com changed: What|Removed |Added CC||jwakely dot gcc at gmail dot ||com http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #1 from paolo dot carlini at oracle dot com 2010-02-26 00:03 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #2 from paolo dot carlini at oracle dot com 2010-02-26 00:13 --- This didn't change in N3035, N3000 had the same wording. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.
--- Comment #3 from paolo dot carlini at oracle dot com 2010-02-26 00:15 --- Chris, can you please double check this? -- paolo dot carlini at oracle dot com changed: What|Removed |Added CC||chris dot fairles at gmail ||dot com http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183
