[Bug libstdc++/43183] New: std::unique_ptr::reset() does not conform to N3035.

[Bug libstdc++/43183] New: std::unique_ptr::reset() does not conform to N3035.

[tjgolubi at netins dot net]

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);
          }
      }


-- 
           Summary: std::unique_ptr::reset() does not conform to N3035.
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: tjgolubi at netins dot net


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183

[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.

[paolo dot carlini at oracle dot com]

------- 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.

[paolo dot carlini at oracle dot com]

------- 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.

[paolo dot carlini at oracle dot com]

------- 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

[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.

[paolo dot carlini at oracle dot com]

------- 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.

[jwakely dot gcc at gmail dot com]

------- 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

[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.

[paolo dot carlini at oracle dot com]

------- 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.

[jwakely dot gcc at gmail dot com]

------- 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.

[jwakely dot gcc at gmail dot com]

------- 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::get<0>(_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.

[paolo dot carlini at oracle dot com]

------- 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|0000-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]

-- 

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.

[tjgolubi at netins dot net]

------- 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::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. 


-- 


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]

------- 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.

[redi at gcc dot gnu dot org]

------- 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

[Bug libstdc++/43183] std::unique_ptr::reset() does not conform to N3035.

[tjgolubi at netins dot net]

------- 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-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. 


-- 


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]

------- 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=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


-- 


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]

------- 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::get<0>(_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.

[jwakely dot gcc at gmail dot com]

------- 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.

[jwakely dot gcc at gmail dot com]

------- 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