Author: sammccall Date: Fri Feb 9 06:42:01 2018 New Revision: 324735 URL: http://llvm.org/viewvc/llvm-project?rev=324735&view=rev Log: [clangd] Collect definitions when indexing.
Within a TU: - as now, collect a declaration from the first occurrence of a symbol (taking clang's canonical declaration) - when we first see a definition occurrence, copy the symbol and add it Across TUs/sources: - mergeSymbol in Merge.h is responsible for combining matching Symbols. This covers dynamic/static merges and cross-TU merges in the static index. - it prefers declarations from Symbols that have a definition. - GlobalSymbolBuilderMain is modified to use mergeSymbol as a reduce step. Random cleanups (can be pulled out): - SymbolFromYAML -> SymbolsFromYAML, new singular SymbolFromYAML added - avoid uninit'd SymbolLocations. Add an idiomatic way to check "absent". - CanonicalDeclaration (as well as Definition) are mapped as optional in YAML. - added operator<< for Symbol & SymbolLocation, for debugging Reviewers: ioeric, hokein Subscribers: klimek, ilya-biryukov, jkorous-apple, cfe-commits Differential Revision: https://reviews.llvm.org/D42942 Modified: clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp clang-tools-extra/trunk/clangd/index/Index.cpp clang-tools-extra/trunk/clangd/index/Index.h clang-tools-extra/trunk/clangd/index/Merge.cpp clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp clang-tools-extra/trunk/clangd/index/SymbolCollector.h clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp clang-tools-extra/trunk/clangd/index/SymbolYAML.h clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Modified: clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp (original) +++ clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp Fri Feb 9 06:42:01 2018 @@ -14,9 +14,11 @@ //===---------------------------------------------------------------------===// #include "index/Index.h" +#include "index/Merge.h" #include "index/SymbolCollector.h" #include "index/SymbolYAML.h" #include "clang/Frontend/FrontendActions.h" +#include "clang/Frontend/CompilerInstance.h" #include "clang/Index/IndexDataConsumer.h" #include "clang/Index/IndexingAction.h" #include "clang/Tooling/CommonOptionsParser.h" @@ -34,6 +36,7 @@ using clang::clangd::SymbolSlab; namespace clang { namespace clangd { +namespace { static llvm::cl::opt<std::string> AssumedHeaderDir( "assume-header-dir", @@ -91,6 +94,25 @@ public: tooling::ExecutionContext *Ctx; }; +// Combine occurrences of the same symbol across translation units. +SymbolSlab mergeSymbols(tooling::ToolResults *Results) { + SymbolSlab::Builder UniqueSymbols; + llvm::BumpPtrAllocator Arena; + Symbol::Details Scratch; + Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) { + Arena.Reset(); + auto Sym = clang::clangd::SymbolFromYAML(Value, Arena); + clang::clangd::SymbolID ID; + Key >> ID; + if (const auto *Existing = UniqueSymbols.find(ID)) + UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch)); + else + UniqueSymbols.insert(Sym); + }); + return std::move(UniqueSymbols).build(); +} + +} // namespace } // namespace clangd } // namespace clang @@ -115,6 +137,7 @@ int main(int argc, const char **argv) { return 1; } + // Map phase: emit symbols found in each translation unit. auto Err = Executor->get()->execute( llvm::make_unique<clang::clangd::SymbolIndexActionFactory>( Executor->get()->getExecutionContext())); @@ -122,22 +145,11 @@ int main(int argc, const char **argv) { llvm::errs() << llvm::toString(std::move(Err)) << "\n"; } - // Deduplicate the result by key and keep the longest value. - // FIXME(ioeric): Merge occurrences, rather than just dropping all but one. - // Definitions and forward declarations have the same key and may both have - // information. Usage count will need to be aggregated across occurrences, - // too. - llvm::StringMap<llvm::StringRef> UniqueSymbols; - Executor->get()->getToolResults()->forEachResult( - [&UniqueSymbols](llvm::StringRef Key, llvm::StringRef Value) { - auto Ret = UniqueSymbols.try_emplace(Key, Value); - if (!Ret.second) { - // If key already exists, keep the longest value. - llvm::StringRef &V = Ret.first->second; - V = V.size() < Value.size() ? Value : V; - } - }); - for (const auto &Sym : UniqueSymbols) - llvm::outs() << Sym.second; + // Reduce phase: combine symbols using the ID as a key. + auto UniqueSymbols = + clang::clangd::mergeSymbols(Executor->get()->getToolResults()); + + // Output phase: emit YAML for result symbols. + SymbolsToYAML(UniqueSymbols, llvm::outs()); return 0; } Modified: clang-tools-extra/trunk/clangd/index/Index.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Index.cpp Fri Feb 9 06:42:01 2018 @@ -10,11 +10,18 @@ #include "Index.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/SHA1.h" +#include "llvm/Support/raw_ostream.h" namespace clang { namespace clangd { using namespace llvm; +raw_ostream &operator<<(raw_ostream &OS, const SymbolLocation &L) { + if (!L) + return OS << "(none)"; + return OS << L.FileURI << "[" << L.StartOffset << "-" << L.EndOffset << "]"; +} + SymbolID::SymbolID(StringRef USR) : HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {} @@ -29,6 +36,10 @@ void operator>>(StringRef Str, SymbolID std::copy(HexString.begin(), HexString.end(), ID.HashValue.begin()); } +raw_ostream &operator<<(raw_ostream &OS, const Symbol &S) { + return OS << S.Scope << S.Name; +} + SymbolSlab::const_iterator SymbolSlab::find(const SymbolID &ID) const { auto It = std::lower_bound(Symbols.begin(), Symbols.end(), ID, [](const Symbol &S, const SymbolID &I) { @@ -55,6 +66,7 @@ static void own(Symbol &S, DenseSet<Stri Intern(S.Name); Intern(S.Scope); Intern(S.CanonicalDeclaration.FileURI); + Intern(S.Definition.FileURI); Intern(S.CompletionLabel); Intern(S.CompletionFilterText); Modified: clang-tools-extra/trunk/clangd/index/Index.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.h (original) +++ clang-tools-extra/trunk/clangd/index/Index.h Fri Feb 9 06:42:01 2018 @@ -27,11 +27,14 @@ struct SymbolLocation { llvm::StringRef FileURI; // The 0-based offset to the first character of the symbol from the beginning // of the source file. - unsigned StartOffset; + unsigned StartOffset = 0; // The 0-based offset to the last character of the symbol from the beginning // of the source file. - unsigned EndOffset; + unsigned EndOffset = 0; + + operator bool() const { return !FileURI.empty(); } }; +llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &); // The class identifies a particular C++ symbol (class, function, method, etc). // @@ -44,7 +47,7 @@ struct SymbolLocation { class SymbolID { public: SymbolID() = default; - SymbolID(llvm::StringRef USR); + explicit SymbolID(llvm::StringRef USR); bool operator==(const SymbolID &Sym) const { return HashValue == Sym.HashValue; @@ -117,13 +120,16 @@ struct Symbol { llvm::StringRef Name; // The containing namespace. e.g. "" (global), "ns::" (top-level namespace). llvm::StringRef Scope; - // The location of the canonical declaration of the symbol. + // The location of the symbol's definition, if one was found. + // This covers the whole definition (e.g. class body). + SymbolLocation Definition; + // The location of the preferred declaration of the symbol. + // This may be the same as Definition. // - // A C++ symbol could have multiple declarations and one definition (e.g. - // a function is declared in ".h" file, and is defined in ".cc" file). - // * For classes, the canonical declaration is usually definition. - // * For non-inline functions, the canonical declaration is a declaration - // (not a definition), which is usually declared in ".h" file. + // A C++ symbol may have multiple declarations, and we pick one to prefer. + // * For classes, the canonical declaration should be the definition. + // * For non-inline functions, the canonical declaration typically appears + // in the ".h" file corresponding to the definition. SymbolLocation CanonicalDeclaration; /// A brief description of the symbol that can be displayed in the completion @@ -160,12 +166,14 @@ struct Symbol { // FIXME: add all occurrences support. // FIXME: add extra fields for index scoring signals. }; +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S); // An immutable symbol container that stores a set of symbols. // The container will maintain the lifetime of the symbols. class SymbolSlab { public: using const_iterator = std::vector<Symbol>::const_iterator; + using iterator = const_iterator; SymbolSlab() = default; Modified: clang-tools-extra/trunk/clangd/index/Merge.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Merge.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Merge.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Merge.cpp Fri Feb 9 06:42:01 2018 @@ -60,32 +60,40 @@ private: Symbol mergeSymbol(const Symbol &L, const Symbol &R, Symbol::Details *Scratch) { assert(L.ID == R.ID); - Symbol S = L; - // For each optional field, fill it from R if missing in L. - // (It might be missing in R too, but that's a no-op). - if (S.CanonicalDeclaration.FileURI == "") - S.CanonicalDeclaration = R.CanonicalDeclaration; + // We prefer information from TUs that saw the definition. + // Classes: this is the def itself. Functions: hopefully the header decl. + // If both did (or both didn't), continue to prefer L over R. + bool PreferR = R.Definition && !L.Definition; + Symbol S = PreferR ? R : L; // The target symbol we're merging into. + const Symbol &O = PreferR ? L : R; // The "other" less-preferred symbol. + + // For each optional field, fill it from O if missing in S. + // (It might be missing in O too, but that's a no-op). + if (!S.Definition) + S.Definition = O.Definition; + if (!S.CanonicalDeclaration) + S.CanonicalDeclaration = O.CanonicalDeclaration; if (S.CompletionLabel == "") - S.CompletionLabel = R.CompletionLabel; + S.CompletionLabel = O.CompletionLabel; if (S.CompletionFilterText == "") - S.CompletionFilterText = R.CompletionFilterText; + S.CompletionFilterText = O.CompletionFilterText; if (S.CompletionPlainInsertText == "") - S.CompletionPlainInsertText = R.CompletionPlainInsertText; + S.CompletionPlainInsertText = O.CompletionPlainInsertText; if (S.CompletionSnippetInsertText == "") - S.CompletionSnippetInsertText = R.CompletionSnippetInsertText; + S.CompletionSnippetInsertText = O.CompletionSnippetInsertText; - if (L.Detail && R.Detail) { - // Copy into scratch space so we can merge. - *Scratch = *L.Detail; - if (Scratch->Documentation == "") - Scratch->Documentation = R.Detail->Documentation; - if (Scratch->CompletionDetail == "") - Scratch->CompletionDetail = R.Detail->CompletionDetail; - S.Detail = Scratch; - } else if (L.Detail) - S.Detail = L.Detail; - else if (R.Detail) - S.Detail = R.Detail; + if (O.Detail) { + if (S.Detail) { + // Copy into scratch space so we can merge. + *Scratch = *S.Detail; + if (Scratch->Documentation == "") + Scratch->Documentation = O.Detail->Documentation; + if (Scratch->CompletionDetail == "") + Scratch->CompletionDetail = O.Detail->CompletionDetail; + S.Detail = Scratch; + } else + S.Detail = O.Detail; + } return S; } Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original) +++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Fri Feb 9 06:42:01 2018 @@ -132,13 +132,19 @@ bool shouldFilterDecl(const NamedDecl *N // For symbols defined inside macros: // * use expansion location, if the symbol is formed via macro concatenation. // * use spelling location, otherwise. +// +// FIXME: EndOffset is inclusive (closed range), and should be exclusive. +// FIXME: Because the underlying ranges are token ranges, this code chops the +// last token in half if it contains multiple characters. +// FIXME: We probably want to get just the location of the symbol name, not +// the whole e.g. class. llvm::Optional<SymbolLocation> -getSymbolLocation(const NamedDecl *D, SourceManager &SM, +getSymbolLocation(const NamedDecl &D, SourceManager &SM, const SymbolCollector::Options &Opts, std::string &FileURIStorage) { - SourceLocation Loc = D->getLocation(); - SourceLocation StartLoc = SM.getSpellingLoc(D->getLocStart()); - SourceLocation EndLoc = SM.getSpellingLoc(D->getLocEnd()); + SourceLocation Loc = D.getLocation(); + SourceLocation StartLoc = SM.getSpellingLoc(D.getLocStart()); + SourceLocation EndLoc = SM.getSpellingLoc(D.getLocEnd()); if (Loc.isMacroID()) { std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM); if (llvm::StringRef(PrintLoc).startswith("<scratch") || @@ -149,8 +155,8 @@ getSymbolLocation(const NamedDecl *D, So // be "<scratch space>" // * symbols controlled and defined by a compile command-line option // `-DName=foo`, the spelling location will be "<command line>". - StartLoc = SM.getExpansionRange(D->getLocStart()).first; - EndLoc = SM.getExpansionRange(D->getLocEnd()).second; + StartLoc = SM.getExpansionRange(D.getLocStart()).first; + EndLoc = SM.getExpansionRange(D.getLocEnd()).second; } } @@ -158,8 +164,11 @@ getSymbolLocation(const NamedDecl *D, So if (!U) return llvm::None; FileURIStorage = std::move(*U); - return SymbolLocation{FileURIStorage, SM.getFileOffset(StartLoc), - SM.getFileOffset(EndLoc)}; + SymbolLocation Result; + Result.FileURI = FileURIStorage; + Result.StartOffset = SM.getFileOffset(StartLoc); + Result.EndOffset = SM.getFileOffset(EndLoc); + return std::move(Result); } } // namespace @@ -195,62 +204,86 @@ bool SymbolCollector::handleDeclOccurenc return true; auto ID = SymbolID(USR); - if (Symbols.find(ID) != nullptr) - return true; + const Symbol* BasicSymbol = Symbols.find(ID); + if (!BasicSymbol) // Regardless of role, ND is the canonical declaration. + BasicSymbol = addDeclaration(*ND, std::move(ID)); + if (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) + addDefinition(*cast<NamedDecl>(ASTNode.OrigD), *BasicSymbol); + } + return true; +} - auto &SM = ND->getASTContext().getSourceManager(); +const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, + SymbolID ID) { + auto &SM = ND.getASTContext().getSourceManager(); + + std::string QName; + llvm::raw_string_ostream OS(QName); + PrintingPolicy Policy(ASTCtx->getLangOpts()); + // Note that inline namespaces are treated as transparent scopes. This + // reflects the way they're most commonly used for lookup. Ideally we'd + // include them, but at query time it's hard to find all the inline + // namespaces to query: the preamble doesn't have a dedicated list. + Policy.SuppressUnwrittenScope = true; + ND.printQualifiedName(OS, Policy); + OS.flush(); + + Symbol S; + S.ID = std::move(ID); + std::tie(S.Scope, S.Name) = splitQualifiedName(QName); + S.SymInfo = index::getSymbolInfo(&ND); + std::string FileURI; + // FIXME: we may want a different "canonical" heuristic than clang chooses. + // Clang seems to choose the first, which may not have the most information. + if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, FileURI)) + S.CanonicalDeclaration = *DeclLoc; - std::string QName; - llvm::raw_string_ostream OS(QName); - PrintingPolicy Policy(ASTCtx->getLangOpts()); - // Note that inline namespaces are treated as transparent scopes. This - // reflects the way they're most commonly used for lookup. Ideally we'd - // include them, but at query time it's hard to find all the inline - // namespaces to query: the preamble doesn't have a dedicated list. - Policy.SuppressUnwrittenScope = true; - ND->printQualifiedName(OS, Policy); - OS.flush(); - - Symbol S; - S.ID = std::move(ID); - std::tie(S.Scope, S.Name) = splitQualifiedName(QName); - S.SymInfo = index::getSymbolInfo(D); - std::string URIStorage; - if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, URIStorage)) - S.CanonicalDeclaration = *DeclLoc; - - // Add completion info. - assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set."); - CodeCompletionResult SymbolCompletion(ND, 0); - const auto *CCS = SymbolCompletion.CreateCodeCompletionString( - *ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator, - *CompletionTUInfo, - /*IncludeBriefComments*/ true); - std::string Label; - std::string SnippetInsertText; - std::string IgnoredLabel; - std::string PlainInsertText; - getLabelAndInsertText(*CCS, &Label, &SnippetInsertText, - /*EnableSnippets=*/true); - getLabelAndInsertText(*CCS, &IgnoredLabel, &PlainInsertText, - /*EnableSnippets=*/false); - std::string FilterText = getFilterText(*CCS); - std::string Documentation = getDocumentation(*CCS); - std::string CompletionDetail = getDetail(*CCS); - - S.CompletionFilterText = FilterText; - S.CompletionLabel = Label; - S.CompletionPlainInsertText = PlainInsertText; - S.CompletionSnippetInsertText = SnippetInsertText; - Symbol::Details Detail; - Detail.Documentation = Documentation; - Detail.CompletionDetail = CompletionDetail; - S.Detail = &Detail; + // Add completion info. + // FIXME: we may want to choose a different redecl, or combine from several. + assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set."); + CodeCompletionResult SymbolCompletion(&ND, 0); + const auto *CCS = SymbolCompletion.CreateCodeCompletionString( + *ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator, + *CompletionTUInfo, + /*IncludeBriefComments*/ true); + std::string Label; + std::string SnippetInsertText; + std::string IgnoredLabel; + std::string PlainInsertText; + getLabelAndInsertText(*CCS, &Label, &SnippetInsertText, + /*EnableSnippets=*/true); + getLabelAndInsertText(*CCS, &IgnoredLabel, &PlainInsertText, + /*EnableSnippets=*/false); + std::string FilterText = getFilterText(*CCS); + std::string Documentation = getDocumentation(*CCS); + std::string CompletionDetail = getDetail(*CCS); + + S.CompletionFilterText = FilterText; + S.CompletionLabel = Label; + S.CompletionPlainInsertText = PlainInsertText; + S.CompletionSnippetInsertText = SnippetInsertText; + Symbol::Details Detail; + Detail.Documentation = Documentation; + Detail.CompletionDetail = CompletionDetail; + S.Detail = &Detail; - Symbols.insert(S); - } + Symbols.insert(S); + return Symbols.find(S.ID); +} - return true; +void SymbolCollector::addDefinition(const NamedDecl &ND, + const Symbol &DeclSym) { + if (DeclSym.Definition) + return; + // If we saw some forward declaration, we end up copying the symbol. + // This is not ideal, but avoids duplicating the "is this a definition" check + // in clang::index. We should only see one definition. + Symbol S = DeclSym; + std::string FileURI; + if (auto DefLoc = getSymbolLocation(ND, ND.getASTContext().getSourceManager(), + Opts, FileURI)) + S.Definition = *DefLoc; + Symbols.insert(S); } } // namespace clangd Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.h?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolCollector.h (original) +++ clang-tools-extra/trunk/clangd/index/SymbolCollector.h Fri Feb 9 06:42:01 2018 @@ -58,6 +58,9 @@ public: SymbolSlab takeSymbols() { return std::move(Symbols).build(); } private: + const Symbol *addDeclaration(const NamedDecl &, SymbolID); + void addDefinition(const NamedDecl &, const Symbol &DeclSymbol); + // All Symbols collected from the AST. SymbolSlab::Builder Symbols; ASTContext *ASTCtx; Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original) +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Fri Feb 9 06:42:01 2018 @@ -97,7 +97,9 @@ template <> struct MappingTraits<Symbol> IO.mapRequired("Name", Sym.Name); IO.mapRequired("Scope", Sym.Scope); IO.mapRequired("SymInfo", Sym.SymInfo); - IO.mapRequired("CanonicalDeclaration", Sym.CanonicalDeclaration); + IO.mapOptional("CanonicalDeclaration", Sym.CanonicalDeclaration, + SymbolLocation()); + IO.mapOptional("Definition", Sym.Definition, SymbolLocation()); IO.mapRequired("CompletionLabel", Sym.CompletionLabel); IO.mapRequired("CompletionFilterText", Sym.CompletionFilterText); IO.mapRequired("CompletionPlainInsertText", Sym.CompletionPlainInsertText); @@ -160,7 +162,7 @@ template <> struct ScalarEnumerationTrai namespace clang { namespace clangd { -SymbolSlab SymbolFromYAML(llvm::StringRef YAMLContent) { +SymbolSlab SymbolsFromYAML(llvm::StringRef YAMLContent) { // Store data of pointer fields (excl. `StringRef`) like `Detail`. llvm::BumpPtrAllocator Arena; llvm::yaml::Input Yin(YAMLContent, &Arena); @@ -173,13 +175,18 @@ SymbolSlab SymbolFromYAML(llvm::StringRe return std::move(Syms).build(); } -std::string SymbolsToYAML(const SymbolSlab& Symbols) { - std::string Str; - llvm::raw_string_ostream OS(Str); +Symbol SymbolFromYAML(llvm::StringRef YAMLContent, + llvm::BumpPtrAllocator &Arena) { + llvm::yaml::Input Yin(YAMLContent, &Arena); + Symbol S; + Yin >> S; + return S; +} + +void SymbolsToYAML(const SymbolSlab& Symbols, llvm::raw_ostream &OS) { llvm::yaml::Output Yout(OS); for (Symbol S : Symbols) // copy: Yout<< requires mutability. Yout << S; - return OS.str(); } std::string SymbolToYAML(Symbol Sym) { Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.h?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolYAML.h (original) +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.h Fri Feb 9 06:42:01 2018 @@ -20,12 +20,17 @@ #include "Index.h" #include "llvm/Support/Error.h" +#include "llvm/Support/raw_ostream.h" namespace clang { namespace clangd { // Read symbols from a YAML-format string. -SymbolSlab SymbolFromYAML(llvm::StringRef YAMLContent); +SymbolSlab SymbolsFromYAML(llvm::StringRef YAMLContent); + +// Read one symbol from a YAML-format string, backed by the arena. +Symbol SymbolFromYAML(llvm::StringRef YAMLContent, + llvm::BumpPtrAllocator &Arena); // Convert a single symbol to YAML-format string. // The YAML result is safe to concatenate. @@ -33,7 +38,7 @@ std::string SymbolToYAML(Symbol Sym); // Convert symbols to a YAML-format string. // The YAML result is safe to concatenate if you have multiple symbol slabs. -std::string SymbolsToYAML(const SymbolSlab& Symbols); +void SymbolsToYAML(const SymbolSlab& Symbols, llvm::raw_ostream &OS); } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original) +++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Fri Feb 9 06:42:01 2018 @@ -37,7 +37,7 @@ std::unique_ptr<SymbolIndex> BuildStatic llvm::errs() << "Can't open " << YamlSymbolFile << "\n"; return nullptr; } - auto Slab = SymbolFromYAML(Buffer.get()->getBuffer()); + auto Slab = SymbolsFromYAML(Buffer.get()->getBuffer()); SymbolSlab::Builder SymsBuilder; for (auto Sym : Slab) SymsBuilder.insert(Sym); Modified: clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp Fri Feb 9 06:42:01 2018 @@ -248,6 +248,28 @@ TEST(MergeTest, Merge) { EXPECT_EQ(M.Detail->Documentation, "--doc--"); } +TEST(MergeTest, PreferSymbolWithDefn) { + Symbol L, R; + Symbol::Details Scratch; + + L.ID = R.ID = SymbolID("hello"); + L.CanonicalDeclaration.FileURI = "file:/left.h"; + R.CanonicalDeclaration.FileURI = "file:/right.h"; + L.CompletionPlainInsertText = "left-insert"; + R.CompletionPlainInsertText = "right-insert"; + + Symbol M = mergeSymbol(L, R, &Scratch); + EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/left.h"); + EXPECT_EQ(M.Definition.FileURI, ""); + EXPECT_EQ(M.CompletionPlainInsertText, "left-insert"); + + R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored. + M = mergeSymbol(L, R, &Scratch); + EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h"); + EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp"); + EXPECT_EQ(M.CompletionPlainInsertText, "right-insert"); +} + } // namespace } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=324735&r1=324734&r2=324735&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Fri Feb 9 06:42:01 2018 @@ -47,12 +47,17 @@ MATCHER_P(Snippet, S, "") { } MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; } MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; } -MATCHER_P(LocationOffsets, Offsets, "") { +MATCHER_P(DeclRange, Offsets, "") { // Offset range in SymbolLocation is [start, end] while in Clangd is [start, // end). + // FIXME: SymbolLocation should be [start, end). return arg.CanonicalDeclaration.StartOffset == Offsets.first && arg.CanonicalDeclaration.EndOffset == Offsets.second - 1; } +MATCHER_P(DefRange, Offsets, "") { + return arg.Definition.StartOffset == Offsets.first && + arg.Definition.EndOffset == Offsets.second - 1; +} namespace clang { namespace clangd { @@ -97,7 +102,8 @@ public: auto Factory = llvm::make_unique<SymbolIndexActionFactory>(CollectorOpts); std::vector<std::string> Args = {"symbol_collector", "-fsyntax-only", - "-std=c++11", TestFileName}; + "-std=c++11", "-include", + TestHeaderName, TestFileName}; Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end()); tooling::ToolInvocation Invocation( Args, @@ -106,14 +112,8 @@ public: InMemoryFileSystem->addFile(TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode)); - - std::string Content = MainCode; - if (!HeaderCode.empty()) - Content = (llvm::Twine("#include\"") + - llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content) - .str(); InMemoryFileSystem->addFile(TestFileName, 0, - llvm::MemoryBuffer::getMemBuffer(Content)); + llvm::MemoryBuffer::getMemBuffer(MainCode)); Invocation.run(); Symbols = Factory->Collector->takeSymbols(); return true; @@ -176,6 +176,42 @@ TEST_F(SymbolCollectorTest, CollectSymbo QName("foo::bar::v2"), QName("foo::baz")})); } +TEST_F(SymbolCollectorTest, Locations) { + // FIXME: the behavior here is bad: chopping tokens, including more than the + // ident range, using half-open ranges. See fixmes in getSymbolLocation(). + CollectorOpts.IndexMainFiles = true; + Annotations Header(R"cpp( + // Declared in header, defined in main. + $xdecl[[extern int X]]; + $clsdecl[[class C]]ls; + $printdecl[[void print()]]; + + // Declared in header, defined nowhere. + $zdecl[[extern int Z]]; + )cpp"); + Annotations Main(R"cpp( + $xdef[[int X = 4]]2; + $clsdef[[class Cls {}]]; + $printdef[[void print() {}]] + + // Declared/defined in main only. + $y[[int Y]]; + )cpp"); + runSymbolCollector(Header.code(), Main.code()); + EXPECT_THAT( + Symbols, + UnorderedElementsAre( + AllOf(QName("X"), DeclRange(Header.offsetRange("xdecl")), + DefRange(Main.offsetRange("xdef"))), + AllOf(QName("Cls"), DeclRange(Header.offsetRange("clsdecl")), + DefRange(Main.offsetRange("clsdef"))), + AllOf(QName("print"), DeclRange(Header.offsetRange("printdecl")), + DefRange(Main.offsetRange("printdef"))), + AllOf(QName("Z"), DeclRange(Header.offsetRange("zdecl"))), + AllOf(QName("Y"), DeclRange(Main.offsetRange("y")), + DefRange(Main.offsetRange("y"))))); +} + TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) { CollectorOpts.IndexMainFiles = false; runSymbolCollector("class Foo {};", /*Main=*/""); @@ -280,10 +316,9 @@ TEST_F(SymbolCollectorTest, SymbolFormed EXPECT_THAT( Symbols, UnorderedElementsAre( - AllOf(QName("abc_Test"), - LocationOffsets(Header.offsetRange("expansion")), + AllOf(QName("abc_Test"), DeclRange(Header.offsetRange("expansion")), DeclURI(TestHeaderURI)), - AllOf(QName("Test"), LocationOffsets(Header.offsetRange("spelling")), + AllOf(QName("Test"), DeclRange(Header.offsetRange("spelling")), DeclURI(TestHeaderURI)))); } @@ -302,13 +337,13 @@ TEST_F(SymbolCollectorTest, SymbolFormed FF2(); )"); runSymbolCollector(/*Header=*/"", Main.code()); - EXPECT_THAT(Symbols, UnorderedElementsAre( - AllOf(QName("abc_Test"), - LocationOffsets(Main.offsetRange("expansion")), - DeclURI(TestFileURI)), - AllOf(QName("Test"), - LocationOffsets(Main.offsetRange("spelling")), - DeclURI(TestFileURI)))); + EXPECT_THAT( + Symbols, + UnorderedElementsAre( + AllOf(QName("abc_Test"), DeclRange(Main.offsetRange("expansion")), + DeclURI(TestFileURI)), + AllOf(QName("Test"), DeclRange(Main.offsetRange("spelling")), + DeclURI(TestFileURI)))); } TEST_F(SymbolCollectorTest, SymbolFormedByCLI) { @@ -322,10 +357,10 @@ TEST_F(SymbolCollectorTest, SymbolFormed runSymbolCollector(Header.code(), /*Main=*/"", /*ExtraArgs=*/{"-DNAME=name"}); - EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf( - QName("name"), - LocationOffsets(Header.offsetRange("expansion")), - DeclURI(TestHeaderURI)))); + EXPECT_THAT(Symbols, + UnorderedElementsAre(AllOf( + QName("name"), DeclRange(Header.offsetRange("expansion")), + DeclURI(TestHeaderURI)))); } TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) { @@ -503,19 +538,23 @@ CompletionSnippetInsertText: 'snippet ... )"; - auto Symbols1 = SymbolFromYAML(YAML1); + auto Symbols1 = SymbolsFromYAML(YAML1); EXPECT_THAT(Symbols1, UnorderedElementsAre(AllOf( QName("clang::Foo1"), Labeled("Foo1-label"), Doc("Foo doc"), Detail("int"), DeclURI("file:///path/foo.h")))); - auto Symbols2 = SymbolFromYAML(YAML2); + auto Symbols2 = SymbolsFromYAML(YAML2); EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf( QName("clang::Foo2"), Labeled("Foo2-label"), Not(HasDetail()), DeclURI("file:///path/bar.h")))); - std::string ConcatenatedYAML = - SymbolsToYAML(Symbols1) + SymbolsToYAML(Symbols2); - auto ConcatenatedSymbols = SymbolFromYAML(ConcatenatedYAML); + std::string ConcatenatedYAML; + { + llvm::raw_string_ostream OS(ConcatenatedYAML); + SymbolsToYAML(Symbols1, OS); + SymbolsToYAML(Symbols2, OS); + } + auto ConcatenatedSymbols = SymbolsFromYAML(ConcatenatedYAML); EXPECT_THAT(ConcatenatedSymbols, UnorderedElementsAre(QName("clang::Foo1"), QName("clang::Foo2"))); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
