[cfe-commits] r126480 - in /cfe/trunk: include/clang/Sema/Overload.h lib/Sema/SemaInit.cpp lib/Sema/SemaOverload.cpp test/SemaCXX/undefined-internal.cpp

[Douglas Gregor]

On Feb 25, 2011, at 12:52 AM, Chandler Carruth wrote:

> Author: chandlerc
> Date: Fri Feb 25 02:52:25 2011
> New Revision: 126480
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=126480&view=rev
> Log:
> Rough fix for PR9323 that prevents Clang from marking copy constructor
> declarations as referenced when in fact we're not going to even form
> a call in the AST. This is significant because we attempt to allow as an
> extension classes with intentionally private and undefined copy
> constructors to have temporaries bound to references, and so shouldn't
> warn about the lack of definition for that copy constructor when the
> class is internal.

Okay, we're avoiding fallout from the extension.

> Doug, John wasn't really satisfied with the presence of overloading at
> all. This is a stop-gap and there may be a better solution. If you can
> give me some hints for how you'd prefer to see this solved, I'll happily
> switch things over.

The MarkDeclarationReferenced is based on a very literal reading of [basic.def.odr]p2:

  A member of a set of candidate functions is odr-used if it is selected by overload resolution when
referred to from a potentially-evaluated expression.

I don't think it's quite as wrong as John thinks it is :)

We could (should?) move this call outside of overload resolution, out to all of the callers that need it. However, it's not likely to actually fix many bugs, since we enter an unevaluated context whenever we're performing overload resolution. For example, Sema::AddMethodCandidate has:

  // Overload resolution is always an unevaluated context.
  EnterExpressionEvaluationContext Unevaluated(*this, Sema::Unevaluated);

as do the other Add*Candidate functions. This would be important even if we moved the MarkDeclarationReferenced elsewhere, because it also suppresses warnings like "passing non-POD value through '...'".

> Modified:
>    cfe/trunk/include/clang/Sema/Overload.h
>    cfe/trunk/lib/Sema/SemaInit.cpp
>    cfe/trunk/lib/Sema/SemaOverload.cpp
>    cfe/trunk/test/SemaCXX/undefined-internal.cpp
> 
> Modified: cfe/trunk/include/clang/Sema/Overload.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=126480&r1=126479&r2=126480&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Overload.h (original)
> +++ cfe/trunk/include/clang/Sema/Overload.h Fri Feb 25 02:52:25 2011
> @@ -653,7 +653,8 @@
>     /// Find the best viable function on this overload set, if it exists.
>     OverloadingResult BestViableFunction(Sema &S, SourceLocation Loc,
>                                          OverloadCandidateSet::iterator& Best,
> -                                         bool UserDefinedConversion = false);
> +                                         bool UserDefinedConversion = false,
> +                                         bool IsExtraneousCopy = false);
> 
>     void NoteCandidates(Sema &S,
>                         OverloadCandidateDisplayKind OCD,
> 
> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=126480&r1=126479&r2=126480&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Feb 25 02:52:25 2011
> @@ -3478,7 +3478,9 @@
>   }
> 
>   OverloadCandidateSet::iterator Best;
> -  switch (CandidateSet.BestViableFunction(S, Loc, Best)) {
> +  switch (CandidateSet.BestViableFunction(S, Loc, Best,
> +                                          /*UserDefinedConversion=*/ false,
> +                                          IsExtraneousCopy)) {
>   case OR_Success:
>     break;
> 
> 
> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=126480&r1=126479&r2=126480&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Feb 25 02:52:25 2011
> @@ -6248,7 +6248,8 @@
> OverloadingResult
> OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
>                                          iterator &Best,
> -                                         bool UserDefinedConversion) {
> +                                         bool UserDefinedConversion,
> +                                         bool IsExtraneousCopy) {
>   // Find the best viable function.
>   Best = end();
>   for (iterator Cand = begin(); Cand != end(); ++Cand) {
> @@ -6286,7 +6287,13 @@
>   //   covers calls to named functions (5.2.2), operator overloading
>   //   (clause 13), user-defined conversions (12.3.2), allocation function for
>   //   placement new (5.3.4), as well as non-default initialization (8.5).
> -  if (Best->Function)
> +  //
> +  // As a special exception, we don't mark functions selected for extraneous
> +  // copy constructor calls as used; the nature of extraneous copy constructor
> +  // calls is that they are never in fact called.
> +  // FIXME: This doesn't seem like the right approach. Should we be doing
> +  // overload resolution at all for extraneous copies?
> +  if (Best->Function && !IsExtraneousCopy)
>     S.MarkDeclarationReferenced(Loc, Best->Function);


Yes, I think we should be doing overload resolution for the extraneous copies, so that we can inform the user that they're thwarting the standard and their code may not be portable.

	- Doug