PR7245: Make rvalue->reference copy errors into warnings.

Adds a -Wrvalue-copy-ctor to control the extwarn.  Still fails when instantiating the call causes other templates to fail to instantitate, but fixing that appears to be too tricky to be worth it.

[chandlerc, 2010-05-31 06:28:15]

Initial comments inline.

Fundamentally, I think these warnings should be disabled by default. From your
comments Jeffery and the linked defect report, I'm quickly being persuaded that
this is in fact a defect in the current standard, and not advisable behavior.
I'm also beginning to think the policy for Clang in general should move toward
warning about 0x incompatibility by-default to further ease the transition to
0x. Thoughts? I may not be able to convince dgregor or sabre, just thought I'd
toss it out there. ;]

Can we leave some bread crumbs about how to turn off instantiation in the event
of these extraneous copies? That also seems important for 0x compat.

http://codereview.appspot.com/1407042/diff/1008/2004
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/1407042/diff/1008/2004#newcode490
include/clang/Basic/DiagnosticSemaKinds.td:490: "calling a
%select{private|protected}0 constructor of class %2; C++98"
This doesn't seem accurate -- are we actually calling the copy constructor?
C++98 only requires it to be accessible, not to be invoked. We could define it
containing assert(false); and it would be fine.

Also, we should say 'copy constructor' here, 'constructor' is too general.

http://codereview.appspot.com/1407042/diff/1008/2004#newcode751
include/clang/Basic/DiagnosticSemaKinds.td:751: def
ext_rvalue_to_reference_temp_copy_no_viable : ExtWarn<
Why do we need two diagnostics? I'm not clear on the difference at least from
reading the diagnostic text.

http://codereview.appspot.com/1407042/diff/1008/2004#newcode755
include/clang/Basic/DiagnosticSemaKinds.td:755: "initializing vector element}0
of type %1; ; C++98 requires an accessible "
Two semicolons.

http://codereview.appspot.com/1407042/diff/1008/2007
File lib/Sema/Sema.h (right):

http://codereview.appspot.com/1407042/diff/1008/2007#newcode2752
lib/Sema/Sema.h:2752: // True when this is an rvalue->reference
I actually think the parameter name better documents this. If you actually want
to limit it to only being used for this one case, we should name the flag after
it: IsRvalueRefBinding or some such.

http://codereview.appspot.com/1407042/diff/1008/2006
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/1407042/diff/1008/2006#newcode3328
lib/Sema/SemaInit.cpp:3328: ? diag::ext_rvalue_to_reference_temp_copy_no_viable
This scares me. Why would public vs. private impact viability??? Access control
should never impact overloading decisions afaik, and a brief read of the
relevant overloading bits of the standard never mentions access. At the very
least, a comment and possibly a standard citation seems warranted here if this
is correctly being degraded to an extension warning...

[Jeffrey Yasskin, 2010-06-02 21:57:08]

On 2010/05/31 06:28:15, chandlerc wrote:
> Initial comments inline.
> 
> Fundamentally, I think these warnings should be disabled by default. From your
> comments Jeffrey and the linked defect report, I'm quickly being persuaded
that
> this is in fact a defect in the current standard, and not advisable behavior.
> I'm also beginning to think the policy for Clang in general should move toward
> warning about 0x incompatibility by-default to further ease the transition to
> 0x. Thoughts? I may not be able to convince dgregor or sabre, just thought I'd
> toss it out there. ;]

As long as we have the -W option, we can disable it even if clang warns by
default. I don't see any 0x-incompatibility here, either way.

> Can we leave some bread crumbs about how to turn off instantiation in the
event
> of these extraneous copies? That also seems important for 0x compat.

I'm not sure what you mean here.

http://codereview.appspot.com/1407042/diff/1008/2004
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/1407042/diff/1008/2004#newcode490
include/clang/Basic/DiagnosticSemaKinds.td:490: "calling a
%select{private|protected}0 constructor of class %2; C++98"
On 2010/05/31 06:28:15, chandlerc wrote:
> This doesn't seem accurate -- are we actually calling the copy constructor?
> C++98 only requires it to be accessible, not to be invoked. We could define it
> containing assert(false); and it would be fine.
> 
> Also, we should say 'copy constructor' here, 'constructor' is too general.

Changed, pending dgregor's corrections.

http://codereview.appspot.com/1407042/diff/1008/2004#newcode751
include/clang/Basic/DiagnosticSemaKinds.td:751: def
ext_rvalue_to_reference_temp_copy_no_viable : ExtWarn<
On 2010/05/31 06:28:15, chandlerc wrote:
> Why do we need two diagnostics? I'm not clear on the difference at least from
> reading the diagnostic text.

That one's for inaccessible constructors, while this one's for things like
"Foo(Foo&)" that can't bind to rvalues at all. The test has several examples.

http://codereview.appspot.com/1407042/diff/1008/2004#newcode755
include/clang/Basic/DiagnosticSemaKinds.td:755: "initializing vector element}0
of type %1; ; C++98 requires an accessible "
On 2010/05/31 06:28:15, chandlerc wrote:
> Two semicolons.

Fixed.

http://codereview.appspot.com/1407042/diff/1008/2007
File lib/Sema/Sema.h (right):

http://codereview.appspot.com/1407042/diff/1008/2007#newcode2752
lib/Sema/Sema.h:2752: // True when this is an rvalue->reference
On 2010/05/31 06:28:15, chandlerc wrote:
> I actually think the parameter name better documents this. If you actually
want
> to limit it to only being used for this one case, we should name the flag
after
> it: IsRvalueRefBinding or some such.

I'll rename the parameter. The diagnostics are specific to rvalue->ref binding.

http://codereview.appspot.com/1407042/diff/1008/2006
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/1407042/diff/1008/2006#newcode3328
lib/Sema/SemaInit.cpp:3328: ? diag::ext_rvalue_to_reference_temp_copy_no_viable
On 2010/05/31 06:28:15, chandlerc wrote:
> This scares me. Why would public vs. private impact viability???

It doesn't. But viability is considered in the rvalue-binding-to-reference check
in C++98, which is what I'm turning into an extwarn. 

> Access control
> should never impact overloading decisions afaik, and a brief read of the
> relevant overloading bits of the standard never mentions access. At the very
> least, a comment and possibly a standard citation seems warranted here if this
> is correctly being degraded to an extension warning...

[Jeffrey Yasskin, 2010-06-02 21:59:32]

I couldn't find a way to use this error in SFINAE, so I didn't bother to keep
that working. If anyone can provide a failing test, I'll be happy to fix it.

[DougGregor, 2010-06-04 23:23:07]

Here's some example code that is well-formed C++98 (since the lack of a suitable
copy constructor kicks the first g() template out of the overload set) but
triggers an ambiguity in C++0x (where we don't look for the copy constructor):

class X {
  X(X&);

public:
  X();
};

template<int> struct int_c { };

template<typename T> T f(const T&);

template<typename T>
int &g(int_c<sizeof(f(T()))> * = 0);

template<typename T> float &g();

void h() {
  float &fp = g<X>();
}

With the changes I've mentioned, please go ahead and commit.

http://codereview.appspot.com/1407042/diff/8001/9002
File include/clang/Basic/DiagnosticGroups.td (right):

http://codereview.appspot.com/1407042/diff/8001/9002#newcode76
include/clang/Basic/DiagnosticGroups.td:76: def RvalueCopyCtor :
DiagGroup<"rvalue-copy-ctor">;
I think that the name "rvalue-copy-ctor" for this warning group is somewhat
misleading; this is about binding a reference to a temporary. I don't have any
great ideas, but perhaps something like "-Wbind-to-temporary-copy"?

http://codereview.appspot.com/1407042/diff/8001/9003
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/1407042/diff/8001/9003#newcode492
include/clang/Basic/DiagnosticSemaKinds.td:492: NoSFINAE,
InGroup<RvalueCopyCtor>;
how about "when binding a reference to a temporary"

http://codereview.appspot.com/1407042/diff/8001/9003#newcode756
include/clang/Basic/DiagnosticSemaKinds.td:756: "constructor when binding an
rvalue to a reference">,
same comment: "when binding a reference to a temporary" ?

http://codereview.appspot.com/1407042/diff/8001/9005
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/1407042/diff/8001/9005#newcode3327
lib/Sema/SemaInit.cpp:3327: S.Diag(Loc, IsExtraneousCopy
Use "IsExtraneousCopy && !S.isSFINAEContext()" to keep the error within SFINAE
context.

http://codereview.appspot.com/1407042/diff/8001/9005#newcode3334
lib/Sema/SemaInit.cpp:3334: return S.ExprError();
You're going to need to make this:

  if (!IsExtraneousCopy || S.isSFINAEContext())
    return S.ExprError();
  return move(CurInit);

As it stands, you've downgraded the error to a warning, but the initialization
code is still returning an error.

[Jeffrey Yasskin, 2010-06-07 06:44:43]

Thanks for the sample code. I've added it to the test. I'll commit once the svn
server is back up.

http://codereview.appspot.com/1407042/diff/8001/9002
File include/clang/Basic/DiagnosticGroups.td (right):

http://codereview.appspot.com/1407042/diff/8001/9002#newcode76
include/clang/Basic/DiagnosticGroups.td:76: def RvalueCopyCtor :
DiagGroup<"rvalue-copy-ctor">;
On 2010/06/04 23:23:08, doug.gregor wrote:
> I think that the name "rvalue-copy-ctor" for this warning group is somewhat
> misleading; this is about binding a reference to a temporary. I don't have any
> great ideas, but perhaps something like "-Wbind-to-temporary-copy"?

Works for me.

http://codereview.appspot.com/1407042/diff/8001/9003
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/1407042/diff/8001/9003#newcode492
include/clang/Basic/DiagnosticSemaKinds.td:492: NoSFINAE,
InGroup<RvalueCopyCtor>;
On 2010/06/04 23:23:08, doug.gregor wrote:
> how about "when binding a reference to a temporary"

Done.

http://codereview.appspot.com/1407042/diff/8001/9003#newcode756
include/clang/Basic/DiagnosticSemaKinds.td:756: "constructor when binding an
rvalue to a reference">,
On 2010/06/04 23:23:08, doug.gregor wrote:
> same comment: "when binding a reference to a temporary" ?

Done.

http://codereview.appspot.com/1407042/diff/8001/9005
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/1407042/diff/8001/9005#newcode3327
lib/Sema/SemaInit.cpp:3327: S.Diag(Loc, IsExtraneousCopy
On 2010/06/04 23:23:08, doug.gregor wrote:
> Use "IsExtraneousCopy && !S.isSFINAEContext()" to keep the error within SFINAE
> context.

Done.

http://codereview.appspot.com/1407042/diff/8001/9005#newcode3334
lib/Sema/SemaInit.cpp:3334: return S.ExprError();
On 2010/06/04 23:23:08, doug.gregor wrote:
> You're going to need to make this:
> 
>   if (!IsExtraneousCopy || S.isSFINAEContext())
>     return S.ExprError();
>   return move(CurInit);
> 
> As it stands, you've downgraded the error to a warning, but the initialization
> code is still returning an error.

Done, and I'm running a bootstrap on the assumption that that's where this would
have broken.