[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).

llvm-svn: 233327

1 files changed, 39 insertions, 25 deletions

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 679419c9c5a..24ba783c4ef 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/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.");