Re: C++11 (abridged version)

[Pedro Alves <palves at redhat dot com>]

On 10/25/2016 09:29 PM, Keith Seitz wrote:
> Hi, Pedro,

Hi Keith!  Thanks for dialing in.

> 
> I'm normally a "go with the flow" kind of person, and I've pretty much
> had my head buried in the compile "sand," but I felt this topic
> important enough to say something -- even if it has already been said.
> 
> TL;DR: I'm *all* for moving directly to C++11. I'm already using quite a
> bit of it on C++ Compile.

How forward-thinking.  :-)

> However, there is one concern I have, and the recent unique_ptr patch
> exemplifies this.  We now have a "unique_ptr." In fact, when using a
> real C++11 compiler, gdb_unique_ptr.h will actually just alias a real
> C++11 std::unique_ptr to gdb::unique_ptr.
> 
> Alas,
> 
>> - Putting gdb::unique_ptr in standard containers is not supported,
>>   since C++03 containers are not move-aware (and our emulation
>>   relies on copy actually moving).
> 
> The comment above shows that while the two are similarly named,
> gdb::unique_ptr isn't yet really a real unique_ptr. 

Right, it's a subset of the API.  Containers are probably where
we actually miss something.  Stateful deleters, etc., not so much.

> However, for
> developers like me who do use a C++11-compliant compiler, that's one
> hidden gotcha waiting to be discovered.

The problem is really on the C++03 containers side more than anything.
There's no way to make C++03 standard containers work correctly with
non-copyable and movable types.  That's why you can't put old
std::auto_ptr in containers either.

boost's unique_ptr emulation supports putting their unique_ptr
in containers, but only because they re-implement the
containers too...  I.e., not in standard container either.

> 
> Fortunately, I read the sources and discovered this before I used
> unique_ptr in containers on the c++compile branch. I'm (mildly)
> concerned that this could too easily lead to yet-another "our own
> dialect" of C++(11) scenario(s).

Yeah...  You just can't do

 std::vector<gdb::unique_ptr <foo>> m_vec;

instead you have to do:

 /* Vector owns objects.  */
 std::vector<foo *> m_vec;

and then destroy the vector's elements yourself, usually
in the destructor of the class m_vec belongs to.

I have examples here (and in other patches of that series):

 https://sourceware.org/ml/gdb-patches/2016-10/msg00531.html

-  /* size of array pointed to by expr_list elt.  */
-  long aexpr_listsize;
-  long next_aexpr_elt;
-  struct agent_expr **aexpr_list;
+  /* Vector owns pointers.  */
+  std::vector<agent_expr *> m_aexprs;
 
...

collection_list::~collection_list ()
{
  for (int ndx = 0; ndx < m_aexprs.size (); ndx++)
    free_agent_expr (m_aexprs[ndx]);
}


So basically you still use gdb::unique_ptr throughout to manage
object's lifetime, and then when "moving" the object to a
container, you release the object out of the unique_ptr
and put the raw pointer in the container.  No way out of
that with C++03, with any kind of owning smart pointer,
not just gdb::unique_ptr.

At:
 https://sourceware.org/ml/gdb-patches/2016-10/msg00537.html

see an example of this moving in action:

  void
 -collection_list::add_aexpr (struct agent_expr *aexpr)
 +collection_list::add_aexpr (agent_expr_up aexpr)
  {
 -  m_aexprs.push_back (aexpr);
 +  m_aexprs.push_back (aexpr.release ());
  }

Actually strictly speaking that can leak if push_back throws
(due to out of memory)  A more correct way to write that would be:

  m_aexprs.push_back (aexpr.get ());
  aexpr.release ()

I didn't bother with that, but maybe I should have.  It
uglifies the code a bit...  I guess we could add a helper
template class for that:

  m_aexprs.push_back (move_to_container (aexpr));

move_to_container would be a function that returns
an instance of a class convertible to the raw pointer type,
in order to pass the raw pointers to push_back,
and, that class would release the smart pointer in its
destructor. The temporary's destructor only runs after
push_back returns.

If we go C++11, the above example can be:

 std::vector<std::unique_ptr <agent_expr>> m_aexprs;

the pushing on the vector is then simply:

  m_aexprs.push_back (std::move (aexpr));

and the collection_list destructor can be completely eliminated..


> 
> Of course, it is *far* too early to make any concrete assertions about
> this. The code has only been in the repository one week.

Yeah.  I think it's not that bad of a trade off.  The container
issue is easy to work around, it's still standard C++, and and, a
unique_ptr put directly in a container would have to pass code review,
while it's very easy to spot.  If it does pass review, it's still
fixable without any algorithmic redesign.  You just have to do
more things manually.

> 
>> #3 - whether we should instead switch to require a C++11 compiler
>>
> 
> We gotta start somewhere, and IMO, C++11 is *by far* the best "safe"
> place to do so.
> 
>> #4 - if so, then what is the policy to accept the next standard
>>      versions going forward.
> 
> I don't know how comfortable I am with such black-and-white rules, but I
> think/hope that maintainers remain *flexible* about decisions like this.
> Do what is "right" when it is "right." [That's not a "rule," per se, but
> what I'd like the "spirit" of the rule to be.]
> 
> I think your offering on this policy indicates that you and I are in
> agreement.

*nod*

> 
> I'm already using C++11 features, and I am absolutely in favor of moving
> directly to C++11.
> 
> TL;SW*
> 
> Keith
> 
> * Too Long; Stopped Writing :-)
> 

Thanks,
Pedro Alves