r242855 - [modules] In C++, stop serializing and deserializing a list of declarations in
Richard Smith
richard-llvm at metafoo.co.uk
Tue Jul 21 16:54:08 PDT 2015
Author: rsmith
Date: Tue Jul 21 18:54:07 2015
New Revision: 242855
URL: http://llvm.org/viewvc/llvm-project?rev=242855&view=rev
Log:
[modules] In C++, stop serializing and deserializing a list of declarations in
the identifier table. This is redundant, since the TU-scope lookups are also
serialized as part of the TU DeclContext, and wasteful in a number of ways. We
still emit the decls for PCH / preamble builds, since for those we want
identical results, not merely semantically equivalent ones.
Modified:
cfe/trunk/include/clang/AST/DeclObjC.h
cfe/trunk/include/clang/Sema/Lookup.h
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaLookup.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CXX/class.access/class.friend/p1.cpp
cfe/trunk/test/CXX/class.access/class.friend/p11.cpp
cfe/trunk/test/Modules/linkage-merge.cpp
Modified: cfe/trunk/include/clang/AST/DeclObjC.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclObjC.h (original)
+++ cfe/trunk/include/clang/AST/DeclObjC.h Tue Jul 21 18:54:07 2015
@@ -1202,13 +1202,8 @@ public:
// might bring in a definition.
// Note: a null value indicates that we don't have a definition and that
// modules are enabled.
- if (!Data.getOpaqueValue()) {
- if (IdentifierInfo *II = getIdentifier()) {
- if (II->isOutOfDate()) {
- updateOutOfDate(*II);
- }
- }
- }
+ if (!Data.getOpaqueValue())
+ getMostRecentDecl();
return Data.getPointer();
}
@@ -1851,13 +1846,8 @@ public:
// might bring in a definition.
// Note: a null value indicates that we don't have a definition and that
// modules are enabled.
- if (!Data.getOpaqueValue()) {
- if (IdentifierInfo *II = getIdentifier()) {
- if (II->isOutOfDate()) {
- updateOutOfDate(*II);
- }
- }
- }
+ if (!Data.getOpaqueValue())
+ getMostRecentDecl();
return Data.getPointer();
}
Modified: cfe/trunk/include/clang/Sema/Lookup.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Lookup.h?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Lookup.h (original)
+++ cfe/trunk/include/clang/Sema/Lookup.h Tue Jul 21 18:54:07 2015
@@ -140,6 +140,7 @@ public:
HideTags(true),
Diagnose(Redecl == Sema::NotForRedeclaration),
AllowHidden(Redecl == Sema::ForRedeclaration),
+ AllowHiddenInternal(AllowHidden),
Shadowed(false)
{
configure();
@@ -162,6 +163,7 @@ public:
HideTags(true),
Diagnose(Redecl == Sema::NotForRedeclaration),
AllowHidden(Redecl == Sema::ForRedeclaration),
+ AllowHiddenInternal(AllowHidden),
Shadowed(false)
{
configure();
@@ -182,6 +184,7 @@ public:
HideTags(Other.HideTags),
Diagnose(false),
AllowHidden(Other.AllowHidden),
+ AllowHiddenInternal(Other.AllowHiddenInternal),
Shadowed(false)
{}
@@ -223,13 +226,20 @@ public:
/// \brief Specify whether hidden declarations are visible, e.g.,
/// for recovery reasons.
void setAllowHidden(bool AH) {
- AllowHidden = AH;
+ AllowHiddenInternal = AllowHidden = AH;
+ }
+
+ /// \brief Specify whether hidden internal declarations are visible.
+ void setAllowHiddenInternal(bool AHI) {
+ AllowHiddenInternal = AHI;
}
/// \brief Determine whether this lookup is permitted to see hidden
/// declarations, such as those in modules that have not yet been imported.
- bool isHiddenDeclarationVisible() const {
- return AllowHidden || LookupKind == Sema::LookupTagName;
+ bool isHiddenDeclarationVisible(NamedDecl *ND) const {
+ return (AllowHidden &&
+ (AllowHiddenInternal || ND->isExternallyVisible())) ||
+ LookupKind == Sema::LookupTagName;
}
/// Sets whether tag declarations should be hidden by non-tag
@@ -302,7 +312,7 @@ public:
if (!D->isInIdentifierNamespace(IDNS))
return nullptr;
- if (isHiddenDeclarationVisible() || isVisible(getSema(), D))
+ if (isHiddenDeclarationVisible(D) || isVisible(getSema(), D))
return D;
return getAcceptableDeclSlow(D);
@@ -511,7 +521,7 @@ public:
/// \brief Change this lookup's redeclaration kind.
void setRedeclarationKind(Sema::RedeclarationKind RK) {
Redecl = RK;
- AllowHidden = (RK == Sema::ForRedeclaration);
+ AllowHiddenInternal = AllowHidden = (RK == Sema::ForRedeclaration);
configure();
}
@@ -678,6 +688,9 @@ private:
/// \brief True if we should allow hidden declarations to be 'visible'.
bool AllowHidden;
+ /// \brief True if we should allow hidden internal declarations to be visible.
+ bool AllowHiddenInternal;
+
/// \brief True if the found declarations were shadowed by some other
/// declaration that we skipped. This only happens when \c LookupKind
/// is \c LookupRedeclarationWithLinkage.
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jul 21 18:54:07 2015
@@ -1796,37 +1796,6 @@ NamedDecl *Sema::LazilyCreateBuiltin(Ide
return New;
}
-/// \brief Filter out any previous declarations that the given declaration
-/// should not consider because they are not permitted to conflict, e.g.,
-/// because they come from hidden sub-modules and do not refer to the same
-/// entity.
-static void filterNonConflictingPreviousDecls(Sema &S,
- NamedDecl *decl,
- LookupResult &previous){
- // This is only interesting when modules are enabled.
- if ((!S.getLangOpts().Modules && !S.getLangOpts().ModulesLocalVisibility) ||
- !S.getLangOpts().ModulesHideInternalLinkage)
- return;
-
- // Empty sets are uninteresting.
- if (previous.empty())
- return;
-
- LookupResult::Filter filter = previous.makeFilter();
- while (filter.hasNext()) {
- NamedDecl *old = filter.next();
-
- // Non-hidden declarations are never ignored.
- if (S.isVisible(old))
- continue;
-
- if (!old->isExternallyVisible())
- filter.erase();
- }
-
- filter.done();
-}
-
/// Typedef declarations don't have linkage, but they still denote the same
/// entity if their types are the same.
/// FIXME: This is notionally doing the same thing as ASTReaderDecl's
@@ -4801,6 +4770,13 @@ NamedDecl *Sema::HandleDeclarator(Scope
LookupResult Previous(*this, NameInfo, LookupOrdinaryName,
ForRedeclaration);
+ // If we're hiding internal-linkage symbols in modules from redeclaration
+ // lookup, let name lookup know.
+ if ((getLangOpts().Modules || getLangOpts().ModulesLocalVisibility) &&
+ getLangOpts().ModulesHideInternalLinkage &&
+ D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_typedef)
+ Previous.setAllowHiddenInternal(false);
+
// See if this is a redefinition of a variable in the same scope.
if (!D.getCXXScopeSpec().isSet()) {
bool IsLinkageLookup = false;
@@ -6500,9 +6476,6 @@ bool Sema::CheckVariableDeclaration(VarD
checkForConflictWithNonVisibleExternC(*this, NewVD, Previous))
Previous.setShadowed();
- // Filter out any non-conflicting previous declarations.
- filterNonConflictingPreviousDecls(*this, NewVD, Previous);
-
if (!Previous.empty()) {
MergeVarDecl(NewVD, Previous);
return true;
@@ -8058,9 +8031,6 @@ bool Sema::CheckFunctionDeclaration(Scop
bool MergeTypeWithPrevious = !getLangOpts().CPlusPlus &&
!Previous.isShadowed();
- // Filter out any non-conflicting previous declarations.
- filterNonConflictingPreviousDecls(*this, NewFD, Previous);
-
bool Redeclaration = false;
NamedDecl *OldDecl = nullptr;
@@ -8114,7 +8084,6 @@ bool Sema::CheckFunctionDeclaration(Scop
// Check for a previous extern "C" declaration with this name.
if (!Redeclaration &&
checkForConflictWithNonVisibleExternC(*this, NewFD, Previous)) {
- filterNonConflictingPreviousDecls(*this, NewFD, Previous);
if (!Previous.empty()) {
// This is an extern "C" declaration with the same name as a previous
// declaration, and thus redeclares that entity...
Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
+++ cfe/trunk/lib/Sema/SemaLookup.cpp Tue Jul 21 18:54:07 2015
@@ -378,7 +378,7 @@ void LookupResult::resolveKind() {
// Don't do any extra resolution if we've already resolved as ambiguous.
if (ResultKind == Ambiguous) return;
- llvm::SmallPtrSet<NamedDecl*, 16> Unique;
+ llvm::SmallDenseMap<NamedDecl*, unsigned, 16> Unique;
llvm::SmallPtrSet<QualType, 16> UniqueTypes;
bool Ambiguous = false;
@@ -414,14 +414,26 @@ void LookupResult::resolveKind() {
}
}
- if (!Unique.insert(D).second) {
+ auto UniqueResult = Unique.insert(std::make_pair(D, I));
+ if (!UniqueResult.second) {
// If it's not unique, pull something off the back (and
// continue at this index).
- // FIXME: This is wrong. We need to take the more recent declaration in
- // order to get the right type, default arguments, etc. We also need to
- // prefer visible declarations to hidden ones (for redeclaration lookup
- // in modules builds).
- Decls[I] = Decls[--N];
+ auto ExistingI = UniqueResult.first->second;
+ auto *Existing = Decls[ExistingI]->getUnderlyingDecl();
+ for (Decl *Prev = Decls[I]->getUnderlyingDecl()->getPreviousDecl(); /**/;
+ Prev = Prev->getPreviousDecl()) {
+ if (Prev == Existing) {
+ // Existing result is older. Replace it with the new one.
+ Decls[ExistingI] = Decls[I];
+ Decls[I] = Decls[--N];
+ break;
+ }
+ if (!Prev) {
+ // New decl is older. Keep the existing one.
+ Decls[I] = Decls[--N];
+ break;
+ }
+ }
continue;
}
Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Tue Jul 21 18:54:07 2015
@@ -735,12 +735,14 @@ ASTIdentifierLookupTraitBase::ReadKey(co
}
/// \brief Whether the given identifier is "interesting".
-static bool isInterestingIdentifier(IdentifierInfo &II, bool IsModule) {
+static bool isInterestingIdentifier(ASTReader &Reader, IdentifierInfo &II,
+ bool IsModule) {
return II.hadMacroDefinition() ||
II.isPoisoned() ||
(IsModule ? II.hasRevertedBuiltin() : II.getObjCOrBuiltinID()) ||
II.hasRevertedTokenIDToIdentifier() ||
- II.getFETokenInfo<void>();
+ (!(IsModule && Reader.getContext().getLangOpts().CPlusPlus) &&
+ II.getFETokenInfo<void>());
}
static bool readBit(unsigned &Bits) {
@@ -767,7 +769,7 @@ IdentifierInfo *ASTIdentifierLookupTrait
}
if (!II->isFromAST()) {
II->setIsFromAST();
- if (isInterestingIdentifier(*II, F.isModule()))
+ if (isInterestingIdentifier(Reader, *II, F.isModule()))
II->setChangedSinceDeserialization();
}
Reader.markIdentifierUpToDate(II);
@@ -5883,10 +5885,13 @@ void ASTReader::CompleteRedeclChain(cons
if (isa<TranslationUnitDecl>(DC) || isa<NamespaceDecl>(DC) ||
isa<CXXRecordDecl>(DC) || isa<EnumDecl>(DC)) {
if (DeclarationName Name = cast<NamedDecl>(D)->getDeclName()) {
- auto *II = Name.getAsIdentifierInfo();
- if (isa<TranslationUnitDecl>(DC) && II) {
+ if (!getContext().getLangOpts().CPlusPlus &&
+ isa<TranslationUnitDecl>(DC)) {
// Outside of C++, we don't have a lookup table for the TU, so update
- // the identifier instead. In C++, either way should work fine.
+ // the identifier instead. (For C++ modules, we don't store decls
+ // in the serialized identifier table, so we do the lookup in the TU.)
+ auto *II = Name.getAsIdentifierInfo();
+ assert(II && "non-identifier name in C?");
if (II->isOutOfDate())
updateOutOfDateIdentifier(*II);
} else
@@ -8443,7 +8448,7 @@ void ASTReader::pushExternalDeclIntoScop
// Remove any fake results before adding any real ones.
auto It = PendingFakeLookupResults.find(II);
if (It != PendingFakeLookupResults.end()) {
- for (auto *ND : PendingFakeLookupResults[II])
+ for (auto *ND : It->second)
SemaObj->IdResolver.RemoveDecl(ND);
// FIXME: this works around module+PCH performance issue.
// Rather than erase the result from the map, which is O(n), just clear
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Jul 21 18:54:07 2015
@@ -3103,6 +3103,7 @@ class ASTIdentifierTableTrait {
Preprocessor &PP;
IdentifierResolver &IdResolver;
bool IsModule;
+ bool NeedDecls;
/// \brief Determines whether this is an "interesting" identifier that needs a
/// full IdentifierInfo structure written into the hash table. Notably, this
@@ -3113,7 +3114,7 @@ class ASTIdentifierTableTrait {
II->isPoisoned() ||
(IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
II->hasRevertedTokenIDToIdentifier() ||
- II->getFETokenInfo<void>())
+ (NeedDecls && II->getFETokenInfo<void>()))
return true;
return false;
@@ -3131,7 +3132,8 @@ public:
ASTIdentifierTableTrait(ASTWriter &Writer, Preprocessor &PP,
IdentifierResolver &IdResolver, bool IsModule)
- : Writer(Writer), PP(PP), IdResolver(IdResolver), IsModule(IsModule) {}
+ : Writer(Writer), PP(PP), IdResolver(IdResolver), IsModule(IsModule),
+ NeedDecls(!IsModule || !Writer.getLangOpts().CPlusPlus) {}
static hash_value_type ComputeHash(const IdentifierInfo* II) {
return llvm::HashString(II->getName());
@@ -3152,10 +3154,12 @@ public:
if (MacroOffset)
DataLen += 4; // MacroDirectives offset.
- for (IdentifierResolver::iterator D = IdResolver.begin(II),
- DEnd = IdResolver.end();
- D != DEnd; ++D)
- DataLen += 4;
+ if (NeedDecls) {
+ for (IdentifierResolver::iterator D = IdResolver.begin(II),
+ DEnd = IdResolver.end();
+ D != DEnd; ++D)
+ DataLen += 4;
+ }
}
using namespace llvm::support;
endian::Writer<little> LE(Out);
@@ -3205,18 +3209,21 @@ public:
if (HadMacroDefinition)
LE.write<uint32_t>(MacroOffset);
- // Emit the declaration IDs in reverse order, because the
- // IdentifierResolver provides the declarations as they would be
- // visible (e.g., the function "stat" would come before the struct
- // "stat"), but the ASTReader adds declarations to the end of the list
- // (so we need to see the struct "stat" before the function "stat").
- // Only emit declarations that aren't from a chained PCH, though.
- SmallVector<NamedDecl *, 16> Decls(IdResolver.begin(II), IdResolver.end());
- for (SmallVectorImpl<NamedDecl *>::reverse_iterator D = Decls.rbegin(),
- DEnd = Decls.rend();
- D != DEnd; ++D)
- LE.write<uint32_t>(
- Writer.getDeclID(getDeclForLocalLookup(PP.getLangOpts(), *D)));
+ if (NeedDecls) {
+ // Emit the declaration IDs in reverse order, because the
+ // IdentifierResolver provides the declarations as they would be
+ // visible (e.g., the function "stat" would come before the struct
+ // "stat"), but the ASTReader adds declarations to the end of the list
+ // (so we need to see the struct "stat" before the function "stat").
+ // Only emit declarations that aren't from a chained PCH, though.
+ SmallVector<NamedDecl *, 16> Decls(IdResolver.begin(II),
+ IdResolver.end());
+ for (SmallVectorImpl<NamedDecl *>::reverse_iterator D = Decls.rbegin(),
+ DEnd = Decls.rend();
+ D != DEnd; ++D)
+ LE.write<uint32_t>(
+ Writer.getDeclID(getDeclForLocalLookup(PP.getLangOpts(), *D)));
+ }
}
};
} // end anonymous namespace
Modified: cfe/trunk/test/CXX/class.access/class.friend/p1.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class.access/class.friend/p1.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/test/CXX/class.access/class.friend/p1.cpp (original)
+++ cfe/trunk/test/CXX/class.access/class.friend/p1.cpp Tue Jul 21 18:54:07 2015
@@ -8,11 +8,12 @@
// friends members of the befriending class.
struct S { static void f(); }; // expected-note 2 {{'S' declared here}}
-S* g() { return 0; } // expected-note 2 {{'g' declared here}}
+S* g() { return 0; }
struct X {
friend struct S;
- friend S* g();
+ friend S* g(); // expected-note 2 {{'g' declared here}}
+ // FIXME: The above two notes would be better attached to line 11.
};
void test1() {
Modified: cfe/trunk/test/CXX/class.access/class.friend/p11.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class.access/class.friend/p11.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/test/CXX/class.access/class.friend/p11.cpp (original)
+++ cfe/trunk/test/CXX/class.access/class.friend/p11.cpp Tue Jul 21 18:54:07 2015
@@ -24,13 +24,12 @@ namespace test2 {
void foo() { // expected-note {{'::test2::foo' declared here}}
struct S1 {
friend void foo(); // expected-error {{no matching function 'foo' found in local scope; did you mean '::test2::foo'?}}
- // expected-note at -1{{'::test2::foo' declared here}}
- // TODO: the above note should go on line 24
};
void foo(); // expected-note {{local declaration nearly matches}}
struct S2 {
- friend void foo();
+ friend void foo(); // expected-note{{'::test2::foo' declared here}}
+ // TODO: the above note should go on line 24
};
{
@@ -48,8 +47,8 @@ namespace test2 {
struct S4 {
friend void bar(); // expected-error {{no matching function 'bar' found in local scope; did you mean '::test2::bar'?}}
- // expected-note at -1 2 {{'::test2::bar' declared here}}
- // TODO: the above two notes should go on line 22
+ // expected-note at -1 {{'::test2::bar' declared here}}
+ // TODO: the above note should go on line 22
};
{ void bar(); }
@@ -82,6 +81,8 @@ namespace test2 {
struct S9 {
struct Inner {
friend void baz(); // expected-error {{no matching function 'baz' found in local scope; did you mean 'bar'?}}
+ // expected-note at -1 {{'::test2::bar' declared here}}
+ // TODO: the above note should go on line 22
};
};
Modified: cfe/trunk/test/Modules/linkage-merge.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/linkage-merge.cpp?rev=242855&r1=242854&r2=242855&view=diff
==============================================================================
--- cfe/trunk/test/Modules/linkage-merge.cpp (original)
+++ cfe/trunk/test/Modules/linkage-merge.cpp Tue Jul 21 18:54:07 2015
@@ -7,9 +7,5 @@ static int f(int);
int f(int);
static void g(int);
-// FIXME: Whether we notice the problem here depends on the order in which we
-// happen to find lookup results for 'g'; LookupResult::resolveKind needs to
-// be taught to prefer a visible result over a non-visible one.
-//
// expected-error at 9 {{functions that differ only in their return type cannot be overloaded}}
// expected-note at Inputs/linkage-merge-foo.h:2 {{previous declaration is here}}
More information about the cfe-commits
mailing list