Skip to content

Commit

Permalink
[clangd] Fix header-guard check for include insertion, and don't inde…
Browse files Browse the repository at this point in the history 
…x header guards.

Summary:
Both of these attempt to check whether a header guard exists while parsing the
file. However the file is only marked as guarded once clang finishes processing
it. We defer the checks and work until SymbolCollector::finish().

This is ugly and ad-hoc, deferring *all* work might be cleaner.

Reviewers: kadircet

Subscribers: ilya-biryukov, MaskRay, jkorous, mgrang, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D61442

llvm-svn: 359880
  • Loading branch information
@sam-mccall
sam-mccall committed May 3, 2019
1 parent 8ff072e commit ec02653
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 43 deletions.
22 changes: 9 additions & 13 deletions clang-tools-extra/clangd/index/Symbol.cpp
View file
Expand Up @@ -48,27 +48,23 @@ static void own(Symbol &S, llvm::UniqueStringSaver &Strings) {
}

void SymbolSlab::Builder::insert(const Symbol &S) {
auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
if (R.second) {
Symbols.push_back(S);
own(Symbols.back(), UniqueStrings);
} else {
auto &Copy = Symbols[R.first->second] = S;
own(Copy, UniqueStrings);
}
own(Symbols[S.ID] = S, UniqueStrings);
}

SymbolSlab SymbolSlab::Builder::build() && {
Symbols = {Symbols.begin(), Symbols.end()}; // Force shrink-to-fit.
// Sort symbols so the slab can binary search over them.
llvm::sort(Symbols,
// Sort symbols into vector so the slab can binary search over them.
std::vector<Symbol> SortedSymbols;
SortedSymbols.reserve(Symbols.size());
for (auto &Entry : Symbols)
SortedSymbols.push_back(std::move(Entry.second));
llvm::sort(SortedSymbols,
[](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
// We may have unused strings from overwritten symbols. Build a new arena.
llvm::BumpPtrAllocator NewArena;
llvm::UniqueStringSaver Strings(NewArena);
for (auto &S : Symbols)
for (auto &S : SortedSymbols)
own(S, Strings);
return SymbolSlab(std::move(NewArena), std::move(Symbols));
return SymbolSlab(std::move(NewArena), std::move(SortedSymbols));
}

} // namespace clangd
Expand Down
12 changes: 7 additions & 5 deletions clang-tools-extra/clangd/index/Symbol.h
View file
Expand Up @@ -204,10 +204,13 @@ class SymbolSlab {
/// This is a deep copy: underlying strings will be owned by the slab.
void insert(const Symbol &S);

/// Returns the symbol with an ID, if it exists. Valid until next insert().
/// Removes the symbol with an ID, if it exists.
void erase(const SymbolID &ID) { Symbols.erase(ID); }

/// Returns the symbol with an ID, if it exists. Valid until insert/remove.
const Symbol *find(const SymbolID &ID) {
auto I = SymbolIndex.find(ID);
return I == SymbolIndex.end() ? nullptr : &Symbols[I->second];
auto I = Symbols.find(ID);
return I == Symbols.end() ? nullptr : &I->second;
}

/// Consumes the builder to finalize the slab.
Expand All @@ -217,9 +220,8 @@ class SymbolSlab {
llvm::BumpPtrAllocator Arena;
/// Intern table for strings. Contents are on the arena.
llvm::UniqueStringSaver UniqueStrings;
std::vector<Symbol> Symbols;
/// Values are indices into Symbols vector.
llvm::DenseMap<SymbolID, size_t> SymbolIndex;
llvm::DenseMap<SymbolID, Symbol> Symbols;
};

private:
Expand Down
64 changes: 40 additions & 24 deletions clang-tools-extra/clangd/index/SymbolCollector.cpp
View file
Expand Up @@ -352,9 +352,8 @@ bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
const auto &SM = PP->getSourceManager();
auto DefLoc = MI->getDefinitionLoc();

// Header guards are not interesting in index. Builtin macros don't have
// useful locations and are not needed for code completions.
if (MI->isUsedForHeaderGuard() || MI->isBuiltinMacro())
// Builtin macros don't have useful locations and aren't needed in completion.
if (MI->isBuiltinMacro())
return true;

// Skip main-file symbols if we are not collecting them.
Expand Down Expand Up @@ -408,22 +407,25 @@ bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
std::string Signature;
std::string SnippetSuffix;
getSignature(*CCS, &Signature, &SnippetSuffix);

std::string Include;
if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
if (auto Header = getIncludeHeader(
Name->getName(), SM.getDecomposedExpansionLoc(DefLoc).first))
Include = std::move(*Header);
}
S.Signature = Signature;
S.CompletionSnippetSuffix = SnippetSuffix;
if (!Include.empty())
S.IncludeHeaders.emplace_back(Include, 1);

IndexedMacros.insert(Name);
setIncludeLocation(S, DefLoc);
Symbols.insert(S);
return true;
}

void SymbolCollector::setIncludeLocation(const Symbol &S,
SourceLocation Loc) {
if (Opts.CollectIncludePath)
if (shouldCollectIncludePath(S.SymInfo.Kind))
// Use the expansion location to get the #include header since this is
// where the symbol is exposed.
IncludeFiles[S.ID] =
PP->getSourceManager().getDecomposedExpansionLoc(Loc).first;
}

void SymbolCollector::finish() {
// At the end of the TU, add 1 to the refcount of all referenced symbols.
auto IncRef = [this](const SymbolID &ID) {
Expand All @@ -440,13 +442,36 @@ void SymbolCollector::finish() {
}
if (Opts.CollectMacro) {
assert(PP);
// First, drop header guards. We can't identify these until EOF.
for (const IdentifierInfo *II : IndexedMacros) {
if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
if (auto ID = getSymbolID(*II, MI, PP->getSourceManager()))
if (MI->isUsedForHeaderGuard())
Symbols.erase(*ID);
}
// Now increment refcounts.
for (const IdentifierInfo *II : ReferencedMacros) {
if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
if (auto ID = getSymbolID(*II, MI, PP->getSourceManager()))
IncRef(*ID);
}
}

// Fill in IncludeHeaders.
// We delay this until end of TU so header guards are all resolved.
// Symbols in slabs aren' mutable, so insert() has to walk all the strings :-(
llvm::SmallString<256> QName;
for (const auto &Entry : IncludeFiles)
if (const Symbol *S = Symbols.find(Entry.first)) {
QName = S->Scope;
QName.append(S->Name);
if (auto Header = getIncludeHeader(QName, Entry.second)) {
Symbol NewSym = *S;
NewSym.IncludeHeaders.push_back({*Header, 1});
Symbols.insert(NewSym);
}
}

const auto &SM = ASTCtx->getSourceManager();
llvm::DenseMap<FileID, std::string> URICache;
auto GetURI = [&](FileID FID) -> llvm::Optional<std::string> {
Expand All @@ -464,7 +489,7 @@ void SymbolCollector::finish() {
}
return Found->second;
};

// Populate Refs slab from DeclRefs.
if (auto MainFileURI = GetURI(SM.getMainFileID())) {
for (const auto &It : DeclRefs) {
if (auto ID = getSymbolID(It.first)) {
Expand Down Expand Up @@ -492,6 +517,7 @@ void SymbolCollector::finish() {
DeclRefs.clear();
FilesToIndexCache.clear();
HeaderIsSelfContainedCache.clear();
IncludeFiles.clear();
}

const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
Expand Down Expand Up @@ -556,17 +582,6 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
std::string ReturnType = getReturnType(*CCS);
S.ReturnType = ReturnType;

std::string Include;
if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
// Use the expansion location to get the #include header since this is
// where the symbol is exposed.
if (auto Header = getIncludeHeader(
QName, SM.getDecomposedExpansionLoc(ND.getLocation()).first))
Include = std::move(*Header);
}
if (!Include.empty())
S.IncludeHeaders.emplace_back(Include, 1);

llvm::Optional<OpaqueType> TypeStorage;
if (S.Flags & Symbol::IndexedForCodeCompletion) {
TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);
Expand All @@ -575,6 +590,7 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
}

Symbols.insert(S);
setIncludeLocation(S, ND.getLocation());
return Symbols.find(S.ID);
}

Expand Down
6 changes: 6 additions & 0 deletions clang-tools-extra/clangd/index/SymbolCollector.h
View file
Expand Up @@ -125,6 +125,12 @@ class SymbolCollector : public index::IndexDataConsumer {

// All Symbols collected from the AST.
SymbolSlab::Builder Symbols;
// File IDs for Symbol.IncludeHeaders.
// The final spelling is calculated in finish().
llvm::DenseMap<SymbolID, FileID> IncludeFiles;
void setIncludeLocation(const Symbol &S, SourceLocation);
// Indexed macros, to be erased if they turned out to be include guards.
llvm::DenseSet<const IdentifierInfo *> IndexedMacros;
// All refs collected from the AST.
// Only symbols declared in preamble (from #include) and referenced from the
// main file will be included.
Expand Down
23 changes: 22 additions & 1 deletion clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
View file
Expand Up @@ -20,6 +20,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock-more-matchers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand All @@ -34,9 +35,9 @@ namespace {
using testing::_;
using testing::AllOf;
using testing::Contains;
using testing::Each;
using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
using testing::Not;
using testing::Pair;
using testing::UnorderedElementsAre;
Expand Down Expand Up @@ -1028,6 +1029,26 @@ TEST_F(SymbolCollectorTest, IncFileInNonHeader) {
Not(IncludeHeader()))));
}

// Features that depend on header-guards are fragile. Header guards are only
// recognized when the file ends, so we have to defer checking for them.
TEST_F(SymbolCollectorTest, HeaderGuardDetected) {
CollectorOpts.CollectIncludePath = true;
CollectorOpts.CollectMacro = true;
runSymbolCollector(R"cpp(
#ifndef HEADER_GUARD_
#define HEADER_GUARD_
// Symbols are seen before the header guard is complete.
#define MACRO
int decl();
#endif // Header guard is recognized here.
)cpp",
"");
EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_"))));
EXPECT_THAT(Symbols, Each(IncludeHeader()));
}

TEST_F(SymbolCollectorTest, NonModularHeader) {
auto TU = TestTU::withHeaderCode("int x();");
EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
Expand Down

0 comments on commit ec02653

Please sign in to comment.