From: Chandler Carruth <chandlerc@gmail.com>
Date: Thu, 26 Mar 2015 22:27:09 +0000 (+0000)
Subject: [Modules] Fix a sneaky bug in r233249 where we would look for implicit
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=283acf13a019076964f3090578e5ad334bc4b576;p=clang

[Modules] Fix a sneaky bug in r233249 where we would look for implicit
constructors in the current lexical context even though name lookup
found them via some other context merged into the redecl chain.

This can only happen for implicit constructors which can only have the
name of the type of the current context, so we can fix this by simply
*always* merging those names first. This also has the advantage of
removing the walk of the current lexical context from the common case
when this is the only constructor name we need to deal with (implicit or
otherwise).

I've enhanced the tests to cover this case (and uncovered an unrelated
bug which I fixed in r233325).

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

diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 679419c9c5..24ba783c4e 100644
--- a/lib/Serialization/ASTWriter.cpp
+++ b/lib/Serialization/ASTWriter.cpp
@@ -3786,35 +3786,49 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
   // Sort the names into a stable order.
   std::sort(Names.begin(), Names.end());
 
-  if (isa<CXXRecordDecl>(DC) &&
-      (!ConstructorNameSet.empty() || !ConversionNameSet.empty())) {
+  if (auto *D = dyn_cast<CXXRecordDecl>(DC)) {
     // We need to establish an ordering of constructor and conversion function
-    // names, and they don't have an intrinsic ordering. So when we have these,
-    // we walk all the names in the decl and add the constructors and
-    // conversion functions which are visible in the order they lexically occur
-    // within the context.
-    for (Decl *ChildD : DC->decls())
-      if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {
-        auto Name = ChildND->getDeclName();
-        switch (Name.getNameKind()) {
-        default:
-          continue;
-
-        case DeclarationName::CXXConstructorName:
-          if (ConstructorNameSet.erase(Name))
-            Names.push_back(Name);
-          break;
+    // names, and they don't have an intrinsic ordering.
+
+    // First we try the easy case by forming the current context's constructor
+    // name and adding that name first. This is a very useful optimization to
+    // avoid walking the lexical declarations in many cases, and it also
+    // handles the only case where a constructor name can come from some other
+    // lexical context -- when that name is an implicit constructor merged from
+    // another declaration in the redecl chain. Any non-implicit constructor or
+    // conversion function which doesn't occur in all the lexical contexts
+    // would be an ODR violation.
+    auto ImplicitCtorName = Context->DeclarationNames.getCXXConstructorName(
+        Context->getCanonicalType(Context->getRecordType(D)));
+    if (ConstructorNameSet.erase(ImplicitCtorName))
+      Names.push_back(ImplicitCtorName);
+
+    // If we still have constructors or conversion functions, we walk all the
+    // names in the decl and add the constructors and conversion functions
+    // which are visible in the order they lexically occur within the context.
+    if (!ConstructorNameSet.empty() || !ConversionNameSet.empty())
+      for (Decl *ChildD : cast<CXXRecordDecl>(DC)->decls())
+        if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {
+          auto Name = ChildND->getDeclName();
+          switch (Name.getNameKind()) {
+          default:
+            continue;
+
+          case DeclarationName::CXXConstructorName:
+            if (ConstructorNameSet.erase(Name))
+              Names.push_back(Name);
+            break;
+
+          case DeclarationName::CXXConversionFunctionName:
+            if (ConversionNameSet.erase(Name))
+              Names.push_back(Name);
+            break;
+          }
 
-        case DeclarationName::CXXConversionFunctionName:
-          if (ConversionNameSet.erase(Name))
-            Names.push_back(Name);
-          break;
+          if (ConstructorNameSet.empty() && ConversionNameSet.empty())
+            break;
         }
 
-        if (ConstructorNameSet.empty() && ConversionNameSet.empty())
-          break;
-      }
-
     assert(ConstructorNameSet.empty() && "Failed to find all of the visible "
                                          "constructors by walking all the "
                                          "lexical members of the context.");
diff --git a/test/Modules/Inputs/stress1/m01.h b/test/Modules/Inputs/stress1/m01.h
index d0b150a738..23a3d4b289 100644
--- a/test/Modules/Inputs/stress1/m01.h
+++ b/test/Modules/Inputs/stress1/m01.h
@@ -3,4 +3,8 @@
 
 #include "common.h"
 
+// Trigger the use of special members for a class this is also defined in other
+// modules.
+inline N00::S01 m01_special_members() { return N00::S01(); }
+
 #endif
diff --git a/test/Modules/Inputs/stress1/merge00.h b/test/Modules/Inputs/stress1/merge00.h
index fc1b4f8118..46d5e41382 100644
--- a/test/Modules/Inputs/stress1/merge00.h
+++ b/test/Modules/Inputs/stress1/merge00.h
@@ -9,6 +9,7 @@
 //#pragma weak pragma_weak01 // expected-warning {{weak identifier 'pragma_weak01' never declared}}
 //#pragma weak pragma_weak04 // expected-warning {{weak identifier 'pragma_waek04' never declared}}
 
+#include "common.h"
 #include "m00.h"
 #include "m01.h"
 #include "m02.h"
@@ -16,6 +17,10 @@
 
 inline int g() { return N00::S00('a').method00('b') + (int)S00(42) + function00(42); }
 
+// Use implicit special memebers again for S01 to ensure that we merge them in
+// successfully from m01.
+inline N00::S01 h() { return N00::S01(); }
+
 #pragma weak pragma_weak02
 #pragma weak pragma_weak05