Re: [PATCH] v2: C++: simplify output from suggest_alternatives_for

[Jason Merrill <jason@redhat.com>]

On Wed, Oct 10, 2018 at 5:01 PM David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2018-10-09 at 18:38 -0400, Jason Merrill wrote:
> > On Tue, Oct 9, 2018 at 1:19 PM David Malcolm <dmalcolm@redhat.com>
> > wrote:
> > > +  /* Emulation of a "move" constructor, but really a copy
> > > +     constructor.  */
> > > +
> > > +  name_hint (const name_hint &other)
> > > +  : m_suggestion (other.m_suggestion),
> > > +    m_deferred (const_cast<name_hint &> (other).take_deferred ())
> > > +  {
> > > +  }
> > > +
> > > +  /* Emulation of "move" assigment, but really copy
> > > assignment.  */
> > > +
> > > +  name_hint& operator= (const name_hint &other)
> > > +  {
> > > +    m_suggestion = other.m_suggestion;
> > > +    m_deferred = const_cast<name_hint &> (other).take_deferred ();
> > > +    return *this;
> > > +  }
> > > +
> > > +  /* Take ownership of this name_hint's deferred_diagnostic, for
> > > use
> > > +     in chaining up deferred diagnostics.  */
> > > +  gnu::unique_ptr<deferred_diagnostic> take_deferred () { return
> > > move (m_deferred); }
> >
> > Why do you want to propagate this hackery into name_hint?  I would
> > expect the defaulted special member functions to do the right thing
> > with m_deferred: in -std=c++98 the implicit copy ops call the
> > gnu::unique_ptr copy ops that actually move, and in -std=c++11 and up
> > we're calling the move constructor for std::unique_ptr, which does
> > the
> > right thing.
> >
> > This also doesn't limit the hack to C++98 mode the way unique-ptr.h
> > does.
> >
> > Jason
>
> Thanks for looking at this.
>
> I ran into issues trying to pass around name_hint instances:
>
> ../../src/gcc/cp/name-lookup.c: In function 'name_hint suggest_alternatives_in_other_namespaces(location_t, tree)':
> ../../src/gcc/cp/name-lookup.c:5591:52: error: use of deleted function 'name_hint::name_hint(const name_hint&)'
> 5591 |   return ns_hints.maybe_decorate_with_limit (result);
>      |                                                    ^
> In file included from ../../src/gcc/cp/name-lookup.c:36:
> ../../src/gcc/c-family/name-hint.h:91:7: note: 'name_hint::name_hint(const name_hint&)' is implicitly deleted because the default definition would be ill-formed:
> 91 | class name_hint
>    |       ^~~~~~~~~
> ../../src/gcc/c-family/name-hint.h:91:7: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = deferred_diagnostic; _Dp = std::default_delete<deferred_diagnostic>]'
> In file included from /home/david/coding/gcc-python/gcc-svn-trunk/install-dogfood/include/c++/9.0.0/memory:80,
>                  from ../../src/gcc/../include/unique-ptr.h:78,
>                  from ../../src/gcc/system.h:730,
>                  from ../../src/gcc/cp/name-lookup.c:23:
> /home/david/coding/gcc-python/gcc-svn-trunk/install-dogfood/include/c++/9.0.0/bits/unique_ptr.h:394:7: note: declared here
> 394 |       unique_ptr(const unique_ptr&) = delete;
>     |       ^~~~~~~~~~
> ../../src/gcc/cp/name-lookup.c:5512:1: note:   initializing argument 1 of 'name_hint namespace_hints::maybe_decorate_with_limit(name_hint)'
> 5512 | namespace_hints::maybe_decorate_with_limit (name_hint hint)
>      | ^~~~~~~~~~~~~~~
>
> I can't use the default copy constructor or assignment operators for an
> object containing a gnu::unique_ptr on C++11, as std::unique_ptr has:
>
>       // Disable copy from lvalue.
>       unique_ptr(const unique_ptr&) = delete;
>       unique_ptr& operator=(const unique_ptr&) = delete;
>
> If I understand things right, in C++11 I should be using move
> construction/move assignment for this.
>
> I can't write "&&" in the function params to explicitly request an
> rvalue-reference, as the code need to be compatible with C++98.
>
> std::move is only defined in C++11 onwards.
>
> Our include/unique-ptr.h defines a gnu::move: for C++11 it's std::move,
> but for C++98 it's only defined for the unique_ptr template.
>
> A solution that seems to work appears to be to define gnu::move for
> C++98 for all types rather than just gnu::unique_ptr, implementing it
> in terms of copying an object via lvalue reference, so that we can
> explicitly request a move using "gnu::move" (==std::move on C++),
> without using C++11 syntax, and falling back to a copy on C++98
> (which effectively moves the ptr from the "victim").
>
> Does that sound sane?

I wouldn't change the unique-ptr.h move to take all types, given that
it copies rather than just passing the reference through, which could
be expensive for unsuspecting users.  And given how it subverts the
C++98 type system, I'd rather explicitly opt into it.  So, let's
overload it for name_hint.  And I'd probably return a reference, e.g.

#if __cplusplus < 201103
// std::move emulation to support the use of gnu::unique_ptr in name_hint.
namespace gnu {
inline const name_hint &
move(name_hint &m) { return m; }
}
#endif

to avoid the unnecessary copy.  Actually, I'd be inclined to do that
for gnu::unique_ptr as well, but would want to make sure that it
doesn't break gdb.

Jason