r307129 - [modules ts] Improve merging of module-private declarations.

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 5 00:47:11 PDT 2017


Author: rsmith
Date: Wed Jul  5 00:47:11 2017
New Revision: 307129

URL: http://llvm.org/viewvc/llvm-project?rev=307129&view=rev
Log:
[modules ts] Improve merging of module-private declarations.

These cases occur frequently for declarations in the global module (above the
module-declaration) in a Modules TS module interface. When we merge a
definition from another module into such a module-private definition, ensure
that we transitively make everything lexically within that definition visible
to that translation unit.

Added:
    cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp
Modified:
    cfe/trunk/include/clang/AST/DeclBase.h
    cfe/trunk/lib/AST/DeclBase.cpp
    cfe/trunk/lib/Sema/SemaLookup.cpp
    cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
    cfe/trunk/lib/Serialization/ASTWriter.cpp

Modified: cfe/trunk/include/clang/AST/DeclBase.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=307129&r1=307128&r2=307129&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclBase.h (original)
+++ cfe/trunk/include/clang/AST/DeclBase.h Wed Jul  5 00:47:11 2017
@@ -749,7 +749,7 @@ public:
   /// Set that this declaration is globally visible, even if it came from a
   /// module that is not visible.
   void setVisibleDespiteOwningModule() {
-    if (getModuleOwnershipKind() == ModuleOwnershipKind::VisibleWhenImported)
+    if (isHidden())
       setModuleOwnershipKind(ModuleOwnershipKind::Visible);
   }
 

Modified: cfe/trunk/lib/AST/DeclBase.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=307129&r1=307128&r2=307129&view=diff
==============================================================================
--- cfe/trunk/lib/AST/DeclBase.cpp (original)
+++ cfe/trunk/lib/AST/DeclBase.cpp Wed Jul  5 00:47:11 2017
@@ -283,8 +283,10 @@ void Decl::setLexicalDeclContext(DeclCon
       setLocalOwningModule(cast<Decl>(DC)->getOwningModule());
   }
 
-  assert((!hasOwningModule() || getOwningModule() || isModulePrivate()) &&
-         "hidden declaration has no owning module");
+  assert(
+      (getModuleOwnershipKind() != ModuleOwnershipKind::VisibleWhenImported ||
+       getOwningModule()) &&
+      "hidden declaration has no owning module");
 }
 
 void Decl::setDeclContextsImpl(DeclContext *SemaDC, DeclContext *LexicalDC,

Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=307129&r1=307128&r2=307129&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
+++ cfe/trunk/lib/Sema/SemaLookup.cpp Wed Jul  5 00:47:11 2017
@@ -1396,6 +1396,13 @@ bool Sema::hasVisibleMergedDefinition(Na
 }
 
 bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) {
+  // FIXME: When not in local visibility mode, we can't tell the difference
+  // between a declaration being visible because we merged a local copy of
+  // the same declaration into it, and it being visible because its owning
+  // module is visible.
+  if (Def->getModuleOwnershipKind() == Decl::ModuleOwnershipKind::Visible &&
+      getLangOpts().ModulesLocalVisibility)
+    return true;
   for (Module *Merged : Context.getModulesWithMergedDefinition(Def))
     if (Merged->getTopLevelModuleName() == getLangOpts().CurrentModule)
       return true;
@@ -1509,25 +1516,33 @@ bool LookupResult::isVisibleSlow(Sema &S
     // FIXME: Don't assume that "same translation unit" means the same thing
     // as "not from an AST file".
     assert(D->isModulePrivate() && "hidden decl has no module");
-    return !D->isFromASTFile();
+    if (!D->isFromASTFile() || SemaRef.hasMergedDefinitionInCurrentModule(D))
+      return true;
+  } else {
+    // If the owning module is visible, and the decl is not module private,
+    // then the decl is visible too. (Module private is ignored within the same
+    // top-level module.)
+    if (D->isModulePrivate()
+          ? DeclModule->getTopLevelModuleName() ==
+                    SemaRef.getLangOpts().CurrentModule ||
+            SemaRef.hasMergedDefinitionInCurrentModule(D)
+          : SemaRef.isModuleVisible(DeclModule) ||
+            SemaRef.hasVisibleMergedDefinition(D))
+      return true;
   }
 
-  // If the owning module is visible, and the decl is not module private,
-  // then the decl is visible too. (Module private is ignored within the same
-  // top-level module.)
-  if (D->isModulePrivate()
-        ? DeclModule->getTopLevelModuleName() ==
-                  SemaRef.getLangOpts().CurrentModule ||
-          SemaRef.hasMergedDefinitionInCurrentModule(D)
-        : SemaRef.isModuleVisible(DeclModule) ||
-          SemaRef.hasVisibleMergedDefinition(D))
-    return true;
+  // Determine whether a decl context is a file context for the purpose of
+  // visibility. This looks through some (export and linkage spec) transparent
+  // contexts, but not others (enums).
+  auto IsEffectivelyFileContext = [](const DeclContext *DC) {
+    return DC->isFileContext() || isa<LinkageSpecDecl>(DC) ||
+           isa<ExportDecl>(DC);
+  };
 
-  // If this declaration is not at namespace scope nor module-private,
+  // If this declaration is not at namespace scope
   // then it is visible if its lexical parent has a visible definition.
   DeclContext *DC = D->getLexicalDeclContext();
-  if (!D->isModulePrivate() && DC && !DC->isFileContext() &&
-      !isa<LinkageSpecDecl>(DC) && !isa<ExportDecl>(DC)) {
+  if (DC && !IsEffectivelyFileContext(DC)) {
     // For a parameter, check whether our current template declaration's
     // lexical context is visible, not whether there's some other visible
     // definition of it, because parameters aren't "within" the definition.
@@ -1535,32 +1550,45 @@ bool LookupResult::isVisibleSlow(Sema &S
     // In C++ we need to check for a visible definition due to ODR merging,
     // and in C we must not because each declaration of a function gets its own
     // set of declarations for tags in prototype scope.
-    if ((D->isTemplateParameter() || isa<ParmVarDecl>(D)
-         || (isa<FunctionDecl>(DC) && !SemaRef.getLangOpts().CPlusPlus))
-            ? isVisible(SemaRef, cast<NamedDecl>(DC))
-            : SemaRef.hasVisibleDefinition(cast<NamedDecl>(DC))) {
-      if (SemaRef.CodeSynthesisContexts.empty() &&
-          // FIXME: Do something better in this case.
-          !SemaRef.getLangOpts().ModulesLocalVisibility) {
-        // Cache the fact that this declaration is implicitly visible because
-        // its parent has a visible definition.
-        D->setVisibleDespiteOwningModule();
-      }
-      return true;
+    bool VisibleWithinParent;
+    if (D->isTemplateParameter() || isa<ParmVarDecl>(D) ||
+        (isa<FunctionDecl>(DC) && !SemaRef.getLangOpts().CPlusPlus))
+      VisibleWithinParent = isVisible(SemaRef, cast<NamedDecl>(DC));
+    else if (D->isModulePrivate()) {
+      // A module-private declaration is only visible if an enclosing lexical
+      // parent was merged with another definition in the current module.
+      VisibleWithinParent = false;
+      do {
+        if (SemaRef.hasMergedDefinitionInCurrentModule(cast<NamedDecl>(DC))) {
+          VisibleWithinParent = true;
+          break;
+        }
+        DC = DC->getLexicalParent();
+      } while (!IsEffectivelyFileContext(DC));
+    } else {
+      VisibleWithinParent = SemaRef.hasVisibleDefinition(cast<NamedDecl>(DC));
     }
-    return false;
+
+    if (VisibleWithinParent && SemaRef.CodeSynthesisContexts.empty() &&
+        // FIXME: Do something better in this case.
+        !SemaRef.getLangOpts().ModulesLocalVisibility) {
+      // Cache the fact that this declaration is implicitly visible because
+      // its parent has a visible definition.
+      D->setVisibleDespiteOwningModule();
+    }
+    return VisibleWithinParent;
   }
 
+  // FIXME: All uses of DeclModule below this point should also check merged
+  // modules.
+  if (!DeclModule)
+    return false;
+
   // Find the extra places where we need to look.
   llvm::DenseSet<Module*> &LookupModules = SemaRef.getLookupModules();
   if (LookupModules.empty())
     return false;
 
-  if (!DeclModule) {
-    DeclModule = SemaRef.getOwningModule(D);
-    assert(DeclModule && "hidden decl not from a module");
-  }
-
   // If our lookup set contains the decl's module, it's visible.
   if (LookupModules.count(DeclModule))
     return true;

Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=307129&r1=307128&r2=307129&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Jul  5 00:47:11 2017
@@ -573,6 +573,8 @@ void ASTDeclReader::VisitDecl(Decl *D) {
       else
         Reader.HiddenNamesMap[Owner].push_back(D);
     }
+  } else if (ModulePrivate) {
+    D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   }
 }
 

Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=307129&r1=307128&r2=307129&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Jul  5 00:47:11 2017
@@ -1462,7 +1462,7 @@ void ASTWriter::WriteControlBlock(Prepro
   }
 
   // Module map file
-  if (WritingModule) {
+  if (WritingModule && WritingModule->Kind == Module::ModuleMapModule) {
     Record.clear();
 
     auto &Map = PP.getHeaderSearchInfo().getModuleMap();

Added: cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp?rev=307129&view=auto
==============================================================================
--- cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp (added)
+++ cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp Wed Jul  5 00:47:11 2017
@@ -0,0 +1,33 @@
+// RUN: rm -f %t
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %s -o %t -DINTERFACE
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DIMPLEMENTATION
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DEARLY_IMPLEMENTATION
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DUSER
+
+// expected-no-diagnostics
+
+#ifdef USER
+import Foo;
+#endif
+
+#ifdef EARLY_IMPLEMENTATION
+module Foo;
+#endif
+
+template<typename T> struct type_template {
+  typedef T type;
+  void f(type);
+};
+
+template<typename T> void type_template<T>::f(type) {}
+
+template<int = 0, typename = int, template<typename> class = type_template>
+struct default_template_args {};
+
+#ifdef INTERFACE
+export module Foo;
+#endif
+
+#ifdef IMPLEMENTATION
+module Foo;
+#endif




More information about the cfe-commits mailing list