ilya-biryukov updated this revision to Diff 148006.
ilya-biryukov added a comment.
Herald added a subscriber: mgorny.
- Move helpers tha switch header and source into separate files.
- Also uprank items coming from the matching headers
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46943
Files:
clangd/AST.cpp
clangd/AST.h
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/MatchingHeader.cpp
clangd/MatchingHeader.h
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -1,5 +1,4 @@
-//===--- TestTU.cpp - Scratch source files for testing ------------*-
-//C++-*-===//
+//===--- TestTU.cpp - Scratch source files for testing -------------------===//
//
// The LLVM Compiler Infrastructure
//
@@ -78,11 +77,11 @@
auto *ND = dyn_cast<NamedDecl>(D);
if (!ND || ND->getNameAsString() != QName)
continue;
- if (Result) {
+ if (Result && ND->getCanonicalDecl() != Result) {
ADD_FAILURE() << "Multiple Decls named " << QName;
assert(false && "QName is not unique");
}
- Result = ND;
+ Result = cast<NamedDecl>(ND->getCanonicalDecl());
}
if (!Result) {
ADD_FAILURE() << "No Decl named " << QName << " in AST";
Index: unittests/clangd/QualityTests.cpp
===================================================================
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -37,15 +37,15 @@
auto AST = Header.build();
SymbolQualitySignals Quality;
- Quality.merge(findSymbol(Symbols, "x"));
+ Quality.merge(findSymbol(Symbols, "x"), /*MainFilename=*/"foo.cpp");
EXPECT_FALSE(Quality.Deprecated);
EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
Symbol F = findSymbol(Symbols, "f");
F.References = 24; // TestTU doesn't count references, so fake it.
Quality = {};
- Quality.merge(F);
+ Quality.merge(F, /*MainFilename=*/"foo.cpp");
EXPECT_FALSE(Quality.Deprecated); // FIXME: Include deprecated bit in index.
EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
EXPECT_EQ(Quality.References, 24u);
@@ -118,6 +118,45 @@
EXPECT_LT(sortText(0, "a"), sortText(0, "z"));
}
+TEST(QualityTests, BoostCurrentFileDecls) {
+ TestTU Test;
+ Test.HeaderFilename = "foo.h";
+ Test.HeaderCode = R"cpp(
+ int test_func_in_header();
+ int test_func_in_header_and_cpp();
+ )cpp";
+ Test.Code = R"cpp(
+ #include "foo.h"
+ int ::test_func_in_header_and_cpp() {
+ }
+ int test_func_in_cpp();
+
+ int test() {
+ test_func_^
+ }
+ )cpp";
+
+ ParsedAST AST = Test.build();
+
+ SymbolQualitySignals FuncInCpp;
+ FuncInCpp.merge(CodeCompletionResult(&findDecl(AST, "test_func_in_cpp"),
+ CCP_Declaration));
+ EXPECT_TRUE(FuncInCpp.AllDeclsInMainFile);
+
+ SymbolQualitySignals FuncInHeader;
+ FuncInHeader.merge(CodeCompletionResult(&findDecl(AST, "test_func_in_header"),
+ CCP_Declaration));
+ EXPECT_FALSE(FuncInHeader.AllDeclsInMainFile);
+
+ SymbolQualitySignals FuncInHeaderAndCpp;
+ FuncInHeaderAndCpp.merge(CodeCompletionResult(
+ &findDecl(AST, "test_func_in_header_and_cpp"), CCP_Declaration));
+ EXPECT_FALSE(FuncInHeaderAndCpp.AllDeclsInMainFile);
+
+ EXPECT_LE(FuncInHeader.evaluate(), FuncInCpp.evaluate());
+ EXPECT_FLOAT_EQ(FuncInHeader.evaluate(), FuncInHeaderAndCpp.evaluate());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Quality.h
===================================================================
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -47,11 +47,13 @@
unsigned SemaCCPriority = 0; // 1-80, 1 is best. 0 means absent.
// FIXME: this is actually a mix of symbol
// quality and relevance. Untangle this.
+ bool AllDeclsInMainFile = true;
+ bool HasDeclsInMatchingHeader = false;
bool Deprecated = false;
unsigned References = 0;
void merge(const CodeCompletionResult &SemaCCResult);
- void merge(const Symbol &IndexResult);
+ void merge(const Symbol &IndexResult, llvm::StringRef MainFilename);
// Condense these signals down to a single number, higher is better.
float evaluate() const;
Index: clangd/Quality.cpp
===================================================================
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -7,7 +7,13 @@
//
//===---------------------------------------------------------------------===//
#include "Quality.h"
+#include "AST.h"
+#include "Logger.h"
+#include "MatchingHeader.h"
+#include "URI.h"
#include "index/Index.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/Sema/CodeCompleteConsumer.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/MathExtras.h"
@@ -17,15 +23,62 @@
namespace clangd {
using namespace llvm;
+namespace {
+struct DeclFiles {
+ bool AllDeclsInMainFile = false;
+ bool HasDeclsInMatchingHeader = false;
+};
+
+static DeclFiles computeDeclFiles(const Decl *D) {
+ DeclFiles R;
+ R.AllDeclsInMainFile = true;
+ R.HasDeclsInMatchingHeader = false;
+
+ const SourceManager &SM = D->getASTContext().getSourceManager();
+ auto MainFile = SM.getFileEntryForID(SM.getMainFileID());
+ assert(MainFile);
+ for (auto *Redecl : D->redecls()) {
+ auto Loc = SM.getSpellingLoc(Redecl->getLocation());
+ if (SM.isWrittenInMainFile(Loc))
+ continue;
+ R.AllDeclsInMainFile = false;
+
+ auto FileName = SM.getFilename(Loc);
+ if (isMainHeaderFor(FileName, MainFile->getName()))
+ R.HasDeclsInMatchingHeader = true;
+ }
+ return R;
+}
+} // namespace
+
void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
SemaCCPriority = SemaCCResult.Priority;
+ if (SemaCCResult.Declaration) {
+ auto DF = computeDeclFiles(SemaCCResult.Declaration);
+ AllDeclsInMainFile &= DF.AllDeclsInMainFile;
+ HasDeclsInMatchingHeader |= DF.HasDeclsInMatchingHeader;
+ }
if (SemaCCResult.Availability == CXAvailability_Deprecated)
Deprecated = true;
}
-void SymbolQualitySignals::merge(const Symbol &IndexResult) {
+void SymbolQualitySignals::merge(const Symbol &IndexResult,
+ llvm::StringRef MainFilename) {
References = std::max(IndexResult.References, References);
+
+ AllDeclsInMainFile = false;
+ if (IndexResult.CanonicalDeclaration.FileURI.empty())
+ return;
+ // FIXME: can we avoid parsing URIs here?
+ auto FileURI = URI::parse(IndexResult.CanonicalDeclaration.FileURI);
+ if (!FileURI) {
+ log(llvm::formatv("Error while parsing URI: {0}",
+ llvm::toString(FileURI.takeError())));
+ return;
+ }
+ if (isMainHeaderFor(FileURI->body(), MainFilename))
+ HasDeclsInMatchingHeader = true;
}
float SymbolQualitySignals::evaluate() const {
@@ -41,6 +94,10 @@
// Priority 80 is a really bad score.
Score *= 2 - std::min<float>(80, SemaCCPriority) / 40;
+ // Things declared in the main file get a large boost.
+ if (HasDeclsInMatchingHeader || AllDeclsInMainFile)
+ Score *= 2;
+
if (Deprecated)
Score *= 0.1f;
Index: clangd/MatchingHeader.h
===================================================================
--- /dev/null
+++ clangd/MatchingHeader.h
@@ -0,0 +1,35 @@
+//===--- MatchingHeader.h - Match source and header files --------*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// Heuristics to match source files with headers and vice versa.
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_MATCHINGHEADER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_MATCHINGHEADER_H
+
+#include "Path.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "llvm/ADT/Optional.h"
+
+namespace clang {
+
+namespace clangd {
+
+/// Returns true iff the heuristics determine that \p Header is a matching
+/// header for a \p Source.
+bool isMainHeaderFor(PathRef Header, PathRef Source);
+
+/// Attempt to find an existing matching header or source file for \p P.
+/// All files accesses are performed via the provided vfs.
+llvm::Optional<Path>
+findMatchingHeaderOrSource(PathRef P,
+ llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS);
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clangd/MatchingHeader.cpp
===================================================================
--- /dev/null
+++ clangd/MatchingHeader.cpp
@@ -0,0 +1,83 @@
+//===--- MatchingHeader.h - Match source and header files -----------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include "MatchingHeader.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+constexpr llvm::StringLiteral SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx",
+ ".c++", ".m", ".mm"};
+constexpr llvm::StringLiteral HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx",
+ ".inc"};
+bool isSourceFile(PathRef P) {
+ StringRef Ext = llvm::sys::path::extension(P);
+ auto SourceIter = std::find_if(
+ std::begin(SourceExtensions), std::end(SourceExtensions),
+ [&Ext](PathRef SourceExt) { return SourceExt.equals_lower(Ext); });
+ return SourceIter != std::end(SourceExtensions);
+}
+
+bool isHeaderFile(PathRef P) {
+ StringRef Ext = llvm::sys::path::extension(P);
+ auto SourceIter = std::find_if(
+ std::begin(HeaderExtensions), std::end(HeaderExtensions),
+ [&Ext](PathRef SourceExt) { return SourceExt.equals_lower(Ext); });
+ return SourceIter != std::end(HeaderExtensions);
+}
+} // namespace
+
+bool isMainHeaderFor(PathRef Header, PathRef Source) {
+ // FIXME: more sophisticated logic could be applied here, e.g. using
+ // clang-format-like regex for matching main files, etc.
+ if (!isSourceFile(Source) || !isHeaderFile(Header))
+ return false;
+ auto SourceNoExt = llvm::sys::path::filename(Source);
+ auto HeaderNoExt = llvm::sys::path::filename(Header);
+ return SourceNoExt.equals_lower(HeaderNoExt);
+}
+
+llvm::Optional<Path>
+findMatchingHeaderOrSource(PathRef P,
+ llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+ bool IsSource = isSourceFile(P);
+ bool IsHeader = isHeaderFile(P);
+ // We can only switch between the known extensions.
+ if (!IsSource && !IsHeader)
+ return llvm::None;
+
+ // Array to lookup extensions for the switch. An opposite of where original
+ // extension was found.
+ ArrayRef<llvm::StringLiteral> NewExts;
+ if (IsSource)
+ NewExts = HeaderExtensions;
+ else
+ NewExts = SourceExtensions;
+
+ // Storage for the new path.
+ SmallString<128> NewPath = P;
+ // Loop through switched extension candidates.
+ for (StringRef NewExt : NewExts) {
+ llvm::sys::path::replace_extension(NewPath, NewExt);
+ if (FS->exists(NewPath))
+ return NewPath.str().str(); // First str() to convert from SmallString to
+ // StringRef, second to convert from StringRef
+ // to std::string
+
+ // Also check NewExt in upper-case, just in case.
+ llvm::sys::path::replace_extension(NewPath, NewExt.upper());
+ if (FS->exists(NewPath))
+ return NewPath.str().str();
+ }
+
+ return llvm::None;
+}
+} // namespace clangd
+} // namespace clang
\ No newline at end of file
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -988,7 +988,7 @@
else
return;
if (IndexResult)
- Quality.merge(*IndexResult);
+ Quality.merge(*IndexResult, FileName);
if (SemaResult) {
Quality.merge(*SemaResult);
Relevance.merge(*SemaResult);
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -11,6 +11,7 @@
#include "CodeComplete.h"
#include "FindSymbols.h"
#include "Headers.h"
+#include "MatchingHeader.h"
#include "SourceCode.h"
#include "XRefs.h"
#include "index/Merge.h"
@@ -307,62 +308,7 @@
}
llvm::Optional<Path> ClangdServer::switchSourceHeader(PathRef Path) {
-
- StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx",
- ".c++", ".m", ".mm"};
- StringRef HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
-
- StringRef PathExt = llvm::sys::path::extension(Path);
-
- // Lookup in a list of known extensions.
- auto SourceIter =
- std::find_if(std::begin(SourceExtensions), std::end(SourceExtensions),
- [&PathExt](PathRef SourceExt) {
- return SourceExt.equals_lower(PathExt);
- });
- bool IsSource = SourceIter != std::end(SourceExtensions);
-
- auto HeaderIter =
- std::find_if(std::begin(HeaderExtensions), std::end(HeaderExtensions),
- [&PathExt](PathRef HeaderExt) {
- return HeaderExt.equals_lower(PathExt);
- });
-
- bool IsHeader = HeaderIter != std::end(HeaderExtensions);
-
- // We can only switch between the known extensions.
- if (!IsSource && !IsHeader)
- return llvm::None;
-
- // Array to lookup extensions for the switch. An opposite of where original
- // extension was found.
- ArrayRef<StringRef> NewExts;
- if (IsSource)
- NewExts = HeaderExtensions;
- else
- NewExts = SourceExtensions;
-
- // Storage for the new path.
- SmallString<128> NewPath = StringRef(Path);
-
- // Instance of vfs::FileSystem, used for file existence checks.
- auto FS = FSProvider.getFileSystem();
-
- // Loop through switched extension candidates.
- for (StringRef NewExt : NewExts) {
- llvm::sys::path::replace_extension(NewPath, NewExt);
- if (FS->exists(NewPath))
- return NewPath.str().str(); // First str() to convert from SmallString to
- // StringRef, second to convert from StringRef
- // to std::string
-
- // Also check NewExt in upper-case, just in case.
- llvm::sys::path::replace_extension(NewPath, NewExt.upper());
- if (FS->exists(NewPath))
- return NewPath.str().str();
- }
-
- return llvm::None;
+ return findMatchingHeaderOrSource(Path, FSProvider.getFileSystem());
}
llvm::Expected<tooling::Replacements>
Index: clangd/CMakeLists.txt
===================================================================
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -22,6 +22,7 @@
FindSymbols.cpp
FuzzyMatch.cpp
GlobalCompilationDatabase.cpp
+ MatchingHeader.cpp
Headers.cpp
JSONExpr.cpp
JSONRPCDispatcher.cpp
Index: clangd/AST.h
===================================================================
--- clangd/AST.h
+++ clangd/AST.h
@@ -26,7 +26,7 @@
///
/// The returned location is usually the spelling location where the name of the
/// decl occurs in the code.
-SourceLocation findNameLoc(const clang::Decl* D);
+SourceLocation findNameLoc(const Decl *D);
} // namespace clangd
} // namespace clang
Index: clangd/AST.cpp
===================================================================
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -17,8 +17,8 @@
namespace clangd {
using namespace llvm;
-SourceLocation findNameLoc(const clang::Decl* D) {
- const auto& SM = D->getASTContext().getSourceManager();
+SourceLocation findNameLoc(const Decl *D) {
+ const auto &SM = D->getASTContext().getSourceManager();
// FIXME: Revisit the strategy, the heuristic is limitted when handling
// macros, we should use the location where the whole definition occurs.
SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
