On Thu, Oct 11, 2018 at 10:28 AM Jason Merrill <ja...@redhat.com> wrote:
>
> On Wed, Oct 10, 2018 at 5:01 PM David Malcolm <dmalc...@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 <dmalc...@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.
And if we made that change to the unique-ptr.h version, i.e.
template<typename T>
const T&
move (T& v)
{
return v;
}
then adding an overload for name_hint wouldn't be necessary.
Jason
