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.

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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@126480 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Sema/Overload.h b/include/clang/Sema/Overload.h
index 3ce3513c21ae87f2426c133908b31a0c8eff3e70..56dec5bba36d752788fc27a3e2bba17a30821ff4 100644 (file)
--- a/include/clang/Sema/Overload.h
+++ b/include/clang/Sema/Overload.h
@@ -653,7 +653,8 @@ namespace clang {
     /// 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,

diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
index 6a1bc97de7970848cf10f2c7d6dd326f3052bf63..d43ce5bb49c994e2b7824857b437b8265248f6c7 100644 (file)
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -3478,7 +3478,9 @@ static ExprResult CopyObject(Sema &S,
   }
 
   OverloadCandidateSet::iterator Best;
-  switch (CandidateSet.BestViableFunction(S, Loc, Best)) {
+  switch (CandidateSet.BestViableFunction(S, Loc, Best,
+                                          /*UserDefinedConversion=*/ false,
+                                          IsExtraneousCopy)) {
   case OR_Success:
     break;
 

diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
index 60873cd969d849834df81186d3ca47599151f33c..c59bfec81161d3114f1ef91a1192b77ec08a8634 100644 (file)
--- a/lib/Sema/SemaOverload.cpp
+++ b/lib/Sema/SemaOverload.cpp
@@ -6248,7 +6248,8 @@ isBetterOverloadCandidate(Sema &S,
 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 @@ OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
   //   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);
 
   return OR_Success;

diff --git a/test/SemaCXX/undefined-internal.cpp b/test/SemaCXX/undefined-internal.cpp
index a7e924996574dbe883c4aec13a48949cf8966aed..e926f18d5ff21105dad435e8da73088f0d0be5ab 100644 (file)
--- a/test/SemaCXX/undefined-internal.cpp
+++ b/test/SemaCXX/undefined-internal.cpp
@@ -105,3 +105,20 @@ namespace test6 {
     a.value = A<Internal>::two;
   }
 }
+
+// We support (as an extension) private, undefined copy constructors when
+// a temporary is bound to a reference even in C++98. Similarly, we shouldn't
+// warn about this copy constructor being used without a definition.
+namespace PR9323 {
+  namespace {
+    struct Uncopyable {
+      Uncopyable() {}
+    private:
+      Uncopyable(const Uncopyable&); // expected-note {{declared private here}}
+    };
+  }
+  void f(const Uncopyable&) {}
+  void test() {
+    f(Uncopyable()); // expected-warning {{C++98 requires an accessible copy constructor}}
+  };
+}