sammccall updated this revision to Diff 145478. sammccall marked 9 inline comments as done. sammccall added a comment.
Address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46524 Files: clang-move/ClangMove.cpp clang-tidy/ClangTidy.cpp clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tidy/ClangTidyOptions.cpp clang-tidy/ClangTidyOptions.h clang-tidy/bugprone/LambdaFunctionNameCheck.cpp clang-tidy/bugprone/MultipleStatementMacroCheck.cpp clang-tidy/google/IntegerTypesCheck.cpp clang-tidy/modernize/RawStringLiteralCheck.cpp clang-tidy/modernize/RawStringLiteralCheck.h clang-tidy/modernize/UseNoexceptCheck.cpp clang-tidy/modernize/UseNullptrCheck.cpp clang-tidy/objc/PropertyDeclarationCheck.cpp clang-tidy/performance/NoexceptMoveConstructorCheck.cpp clang-tidy/tool/ClangTidyMain.cpp clangd/AST.cpp clangd/CMakeLists.txt clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/CodeComplete.cpp clangd/Protocol.h clangd/Quality.cpp clangd/Quality.h clangd/SourceCode.cpp clangd/SourceCode.h clangd/XRefs.cpp clangd/XRefs.h clangd/index/FileIndex.cpp clangd/index/FileIndex.h clangd/index/Index.cpp clangd/index/Index.h clangd/index/MemIndex.cpp clangd/index/SymbolCollector.cpp docs/ReleaseNotes.rst docs/clang-tidy/index.rst test/clang-tidy/google-runtime-int.cpp test/clang-tidy/modernize-raw-string-literal.cpp test/clang-tidy/objc-property-declaration.m test/clang-tidy/temporaries.cpp test/clangd/rename.test unittests/clang-tidy/ClangTidyOptionsTest.cpp unittests/clangd/CMakeLists.txt unittests/clangd/ClangdUnitTests.cpp unittests/clangd/CodeCompleteTests.cpp unittests/clangd/DraftStoreTests.cpp unittests/clangd/FileIndexTests.cpp unittests/clangd/QualityTests.cpp unittests/clangd/SourceCodeTests.cpp unittests/clangd/SymbolCollectorTests.cpp unittests/clangd/TestFS.cpp unittests/clangd/TestTU.cpp unittests/clangd/TestTU.h unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -12,10 +12,11 @@ #include "Matchers.h" #include "SyncAPI.h" #include "TestFS.h" +#include "TestTU.h" #include "XRefs.h" -#include "clang/Frontend/CompilerInvocation.h" -#include "clang/Frontend/PCHContainerOperations.h" -#include "clang/Frontend/Utils.h" +#include "index/FileIndex.h" +#include "index/SymbolCollector.h" +#include "clang/Index/IndexingAction.h" #include "llvm/Support/Path.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -36,18 +37,6 @@ std::vector<Diag> Diagnostics) override {} }; -// FIXME: this is duplicated with FileIndexTests. Share it. -ParsedAST build(StringRef Code) { - auto CI = createInvocationFromCommandLine( - {"clang", "-xc++", testPath("Foo.cpp").c_str()}); - auto Buf = MemoryBuffer::getMemBuffer(Code); - auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf), - std::make_shared<PCHContainerOperations>(), - vfs::getRealFileSystem()); - assert(AST.hasValue()); - return std::move(*AST); -} - // Extracts ranges from an annotated example, and constructs a matcher for a // highlight set. Ranges should be named $read/$write as appropriate. Matcher<const std::vector<DocumentHighlight> &> @@ -98,14 +87,75 @@ }; for (const char *Test : Tests) { Annotations T(Test); - auto AST = build(T.code()); + auto AST = TestTU::withCode(T.code()).build(); EXPECT_THAT(findDocumentHighlights(AST, T.point()), HighlightsFrom(T)) << Test; } } MATCHER_P(RangeIs, R, "") { return arg.range == R; } +TEST(GoToDefinition, WithIndex) { + Annotations SymbolHeader(R"cpp( + class $forward[[Forward]]; + class $foo[[Foo]] {}; + + void $f1[[f1]](); + + inline void $f2[[f2]]() {} + )cpp"); + Annotations SymbolCpp(R"cpp( + class $forward[[forward]] {}; + void $f1[[f1]]() {} + )cpp"); + + TestTU TU; + TU.Code = SymbolCpp.code(); + TU.HeaderCode = SymbolHeader.code(); + auto Index = TU.index(); + auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) { + auto AST = TestTU::withCode(Main.code()).build(); + return clangd::findDefinitions(AST, Main.point(), Index.get()); + }; + + Annotations Test(R"cpp(// only declaration in AST. + void [[f1]](); + int main() { + ^f1(); + } + )cpp"); + EXPECT_THAT(runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray( + {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())})); + + Test = Annotations(R"cpp(// definition in AST. + void [[f1]]() {} + int main() { + ^f1(); + } + )cpp"); + EXPECT_THAT(runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray( + {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))})); + + Test = Annotations(R"cpp(// forward declaration in AST. + class [[Foo]]; + F^oo* create(); + )cpp"); + EXPECT_THAT(runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray( + {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())})); + + Test = Annotations(R"cpp(// defintion in AST. + class [[Forward]] {}; + F^orward create(); + )cpp"); + EXPECT_THAT(runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray({ + RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")), + })); +} + TEST(GoToDefinition, All) { const char *Tests[] = { R"cpp(// Local variable @@ -251,7 +301,7 @@ }; for (const char *Test : Tests) { Annotations T(Test); - auto AST = build(T.code()); + auto AST = TestTU::withCode(T.code()).build(); std::vector<Matcher<Location>> ExpectedLocations; for (const auto &R : T.ranges()) ExpectedLocations.push_back(RangeIs(R)); @@ -583,7 +633,7 @@ for (const OneTest &Test : Tests) { Annotations T(Test.Input); - auto AST = build(T.code()); + auto AST = TestTU::withCode(T.code()).build(); Hover H = getHover(AST, T.point()); EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input; Index: unittests/clangd/TestTU.h =================================================================== --- /dev/null +++ unittests/clangd/TestTU.h @@ -0,0 +1,59 @@ +//===--- TestTU.h - Scratch source files for testing ------------*- C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// +// +// Many tests for indexing, code completion etc are most naturally expressed +// using code examples. +// TestTU lets test define these examples in a common way without dealing with +// the mechanics of VFS and compiler interactions, and then easily grab the +// AST, particular symbols, etc. +// +//===---------------------------------------------------------------------===// +#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H +#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H +#include "ClangdUnit.h" +#include "index/Index.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { + +struct TestTU { + static TestTU withCode(llvm::StringRef Code) { + TestTU TU; + TU.Code = Code; + return TU; + } + + static TestTU withHeaderCode(llvm::StringRef HeaderCode) { + TestTU TU; + TU.HeaderCode = HeaderCode; + return TU; + } + + // The code to be compiled. + std::string Code; + std::string Filename = "TestTU.cpp"; + + // Define contents of a header to be included by TestTU.cpp. + std::string HeaderCode; + std::string HeaderFilename = "TestTU.h"; + + ParsedAST build() const; + SymbolSlab headerSymbols() const; + std::unique_ptr<SymbolIndex> index() const; +}; + +// Look up an index symbol by qualified name, which must be unique. +const Symbol &findSymbol(const SymbolSlab &, llvm::StringRef QName); +// Look up an AST symbol by qualified name, which must be unique and top-level. +const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName); + +} // namespace clangd +} // namespace clang +#endif Index: unittests/clangd/TestTU.cpp =================================================================== --- /dev/null +++ unittests/clangd/TestTU.cpp @@ -0,0 +1,95 @@ +//===--- TestTU.cpp - Scratch source files for testing ------------*- +//C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// +#include "TestTU.h" +#include "TestFS.h" +#include "index/FileIndex.h" +#include "index/MemIndex.h" +#include "clang/Frontend/CompilerInvocation.h" +#include "clang/Frontend/PCHContainerOperations.h" +#include "clang/Frontend/Utils.h" + +namespace clang { +namespace clangd { +using namespace llvm; + +ParsedAST TestTU::build() const { + std::string FullFilename = testPath(Filename), + FullHeaderName = testPath(HeaderFilename); + std::vector<const char *> Cmd = {"clang", FullFilename.c_str()}; + // FIXME: this shouldn't need to be conditional, but it breaks a + // GoToDefinition test for some reason (getMacroArgExpandedLocation fails). + if (!HeaderCode.empty()) { + Cmd.push_back("-include"); + Cmd.push_back(FullHeaderName.c_str()); + } + auto AST = ParsedAST::Build( + createInvocationFromCommandLine(Cmd), nullptr, + MemoryBuffer::getMemBufferCopy(Code), + std::make_shared<PCHContainerOperations>(), + buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}})); + if (!AST.hasValue()) { + ADD_FAILURE() << "Failed to build code:\n" << Code; + llvm_unreachable("Failed to build TestTU!"); + } + return std::move(*AST); +} + +SymbolSlab TestTU::headerSymbols() const { + auto AST = build(); + return indexAST(&AST); +} + +std::unique_ptr<SymbolIndex> TestTU::index() const { + return MemIndex::build(headerSymbols()); +} + +// Look up a symbol by qualified name, which must be unique. +const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) { + const Symbol *Result = nullptr; + for (const Symbol &S : Slab) { + if (QName != (S.Scope + S.Name).str()) + continue; + if (Result) { + ADD_FAILURE() << "Multiple symbols named " << QName << ":\n" + << *Result << "\n---\n" + << S; + assert(false && "QName is not unique"); + } + Result = &S; + } + if (!Result) { + ADD_FAILURE() << "No symbol named " << QName << " in " + << ::testing::PrintToString(Slab); + assert(false && "No symbol with QName"); + } + return *Result; +} + +const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName) { + const NamedDecl *Result = nullptr; + for (const Decl *D : AST.getTopLevelDecls()) { + auto *ND = dyn_cast<NamedDecl>(D); + if (!ND || ND->getNameAsString() != QName) + continue; + if (Result) { + ADD_FAILURE() << "Multiple Decls named " << QName; + assert(false && "QName is not unique"); + } + Result = ND; + } + if (!Result) { + ADD_FAILURE() << "No Decl named " << QName << " in AST"; + assert(false && "No Decl with QName"); + } + return *Result; +} + +} // namespace clangd +} // namespace clang Index: unittests/clangd/TestFS.cpp =================================================================== --- unittests/clangd/TestFS.cpp +++ unittests/clangd/TestFS.cpp @@ -20,8 +20,8 @@ new vfs::InMemoryFileSystem); for (auto &FileAndContents : Files) { MemFS->addFile(FileAndContents.first(), time_t(), - MemoryBuffer::getMemBuffer(FileAndContents.second, - FileAndContents.first())); + MemoryBuffer::getMemBufferCopy(FileAndContents.second, + FileAndContents.first())); } return MemFS; } Index: unittests/clangd/SymbolCollectorTests.cpp =================================================================== --- unittests/clangd/SymbolCollectorTests.cpp +++ unittests/clangd/SymbolCollectorTests.cpp @@ -689,6 +689,15 @@ IncludeHeader(TestHeaderURI), DefURI(TestFileURI)))); } +TEST_F(SymbolCollectorTest, UTF16Character) { + // ö is 2-bytes. + Annotations Header(/*Header=*/"class [[pörk]] {};"); + runSymbolCollector(Header.code(), /*Main=*/""); + EXPECT_THAT(Symbols, UnorderedElementsAre( + AllOf(QName("pörk"), DeclRange(Header.range())))); +} + + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/SourceCodeTests.cpp =================================================================== --- unittests/clangd/SourceCodeTests.cpp +++ unittests/clangd/SourceCodeTests.cpp @@ -24,9 +24,10 @@ return arg.line == Line && arg.character == Col; } +// The = → 🡆 below are ASCII (1 byte), BMP (3 bytes), and astral (4 bytes). const char File[] = R"(0:0 = 0 -1:0 = 8 -2:0 = 16)"; +1:0 → 8 +2:0 🡆 18)"; /// A helper to make tests easier to read. Position position(int line, int character) { @@ -66,25 +67,31 @@ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3), false), HasValue(11)); EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 6)), - HasValue(14)); // last character + HasValue(16)); // last character EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 7)), - HasValue(15)); // the newline itself + HasValue(17)); // the newline itself EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8)), - HasValue(15)); // out of range + HasValue(17)); // out of range EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8), false), Failed()); // out of range // last line EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, -1)), Failed()); // out of range EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 0)), - HasValue(16)); // first character + HasValue(18)); // first character EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 3)), - HasValue(19)); // middle character - EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 7)), - HasValue(23)); // last character + HasValue(21)); // middle character + EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 5), false), + Failed()); // middle of surrogate pair + EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 5)), + HasValue(26)); // middle of surrogate pair + EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 6), false), + HasValue(26)); // end of surrogate pair EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 8)), - HasValue(24)); // EOF - EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 9), false), + HasValue(28)); // last character + EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 9)), + HasValue(29)); // EOF + EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 10), false), Failed()); // out of range // line out of bounds EXPECT_THAT_EXPECTED(positionToOffset(File, position(3, 0)), Failed()); @@ -97,14 +104,19 @@ EXPECT_THAT(offsetToPosition(File, 6), Pos(0, 6)) << "end of first line"; EXPECT_THAT(offsetToPosition(File, 7), Pos(0, 7)) << "first newline"; EXPECT_THAT(offsetToPosition(File, 8), Pos(1, 0)) << "start of second line"; - EXPECT_THAT(offsetToPosition(File, 11), Pos(1, 3)) << "in second line"; - EXPECT_THAT(offsetToPosition(File, 14), Pos(1, 6)) << "end of second line"; - EXPECT_THAT(offsetToPosition(File, 15), Pos(1, 7)) << "second newline"; - EXPECT_THAT(offsetToPosition(File, 16), Pos(2, 0)) << "start of last line"; - EXPECT_THAT(offsetToPosition(File, 19), Pos(2, 3)) << "in last line"; - EXPECT_THAT(offsetToPosition(File, 23), Pos(2, 7)) << "end of last line"; - EXPECT_THAT(offsetToPosition(File, 24), Pos(2, 8)) << "EOF"; - EXPECT_THAT(offsetToPosition(File, 25), Pos(2, 8)) << "out of bounds"; + EXPECT_THAT(offsetToPosition(File, 12), Pos(1, 4)) << "before BMP char"; + EXPECT_THAT(offsetToPosition(File, 13), Pos(1, 5)) << "in BMP char"; + EXPECT_THAT(offsetToPosition(File, 15), Pos(1, 5)) << "after BMP char"; + EXPECT_THAT(offsetToPosition(File, 16), Pos(1, 6)) << "end of second line"; + EXPECT_THAT(offsetToPosition(File, 17), Pos(1, 7)) << "second newline"; + EXPECT_THAT(offsetToPosition(File, 18), Pos(2, 0)) << "start of last line"; + EXPECT_THAT(offsetToPosition(File, 21), Pos(2, 3)) << "in last line"; + EXPECT_THAT(offsetToPosition(File, 22), Pos(2, 4)) << "before astral char"; + EXPECT_THAT(offsetToPosition(File, 24), Pos(2, 6)) << "in astral char"; + EXPECT_THAT(offsetToPosition(File, 26), Pos(2, 6)) << "after astral char"; + EXPECT_THAT(offsetToPosition(File, 28), Pos(2, 8)) << "end of last line"; + EXPECT_THAT(offsetToPosition(File, 29), Pos(2, 9)) << "EOF"; + EXPECT_THAT(offsetToPosition(File, 30), Pos(2, 9)) << "out of bounds"; } } // namespace Index: unittests/clangd/QualityTests.cpp =================================================================== --- /dev/null +++ unittests/clangd/QualityTests.cpp @@ -0,0 +1,123 @@ +//===-- SourceCodeTests.cpp ------------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// Evaluating scoring functions isn't a great fit for assert-based tests. +// For interesting cases, both exact scores and "X beats Y" are too brittle to +// make good hard assertions. +// +// Here we test the signal extraction and sanity-check that signals point in +// the right direction. This should be supplemented by quality metrics which +// we can compute from a corpus of queries and preferred rankings. +// +//===----------------------------------------------------------------------===// + +#include "Quality.h" +#include "TestTU.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace { + +TEST(QualityTests, SymbolQualitySignalExtraction) { + auto Header = TestTU::withHeaderCode(R"cpp( + int x; + + [[deprecated]] + int f() { return x; } + )cpp"); + auto Symbols = Header.headerSymbols(); + auto AST = Header.build(); + + SymbolQualitySignals Quality; + Quality.merge(findSymbol(Symbols, "x")); + 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); + EXPECT_FALSE(Quality.Deprecated); // FIXME: Include deprecated bit in index. + EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority); + EXPECT_EQ(Quality.References, 24u); + + Quality = {}; + Quality.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42)); + EXPECT_TRUE(Quality.Deprecated); + EXPECT_EQ(Quality.SemaCCPriority, 42u); + EXPECT_EQ(Quality.References, SymbolQualitySignals().References); +} + +TEST(QualityTests, SymbolRelevanceSignalExtraction) { + auto AST = TestTU::withHeaderCode(R"cpp( + [[deprecated]] + int f() { return 0; } + )cpp") + .build(); + + SymbolRelevanceSignals Relevance; + Relevance.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42, + nullptr, false, /*Accessible=*/false)); + EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch); + EXPECT_TRUE(Relevance.Forbidden); +} + +// Do the signals move the scores in the direction we expect? +TEST(QualityTests, SymbolQualitySignalsSanity) { + SymbolQualitySignals Default; + EXPECT_EQ(Default.evaluate(), 1); + + SymbolQualitySignals Deprecated; + Deprecated.Deprecated = true; + EXPECT_LT(Deprecated.evaluate(), Default.evaluate()); + + SymbolQualitySignals WithReferences, ManyReferences; + WithReferences.References = 10; + ManyReferences.References = 1000; + EXPECT_GT(WithReferences.evaluate(), Default.evaluate()); + EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate()); + + SymbolQualitySignals LowPriority, HighPriority; + LowPriority.SemaCCPriority = 60; + HighPriority.SemaCCPriority = 20; + EXPECT_GT(HighPriority.evaluate(), Default.evaluate()); + EXPECT_LT(LowPriority.evaluate(), Default.evaluate()); +} + +TEST(QualityTests, SymbolRelevanceSignalsSanity) { + SymbolRelevanceSignals Default; + EXPECT_EQ(Default.evaluate(), 1); + + SymbolRelevanceSignals Forbidden; + Forbidden.Forbidden = true; + EXPECT_LT(Forbidden.evaluate(), Default.evaluate()); + + SymbolRelevanceSignals PoorNameMatch; + PoorNameMatch.NameMatch = 0.2; + EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate()); +} + +TEST(QualityTests, SortText) { + EXPECT_LT(sortText(std::numeric_limits<float>::infinity()), sortText(1000.2)); + EXPECT_LT(sortText(1000.2), sortText(1)); + EXPECT_LT(sortText(1), sortText(0.3)); + EXPECT_LT(sortText(0.3), sortText(0)); + EXPECT_LT(sortText(0), sortText(-10)); + EXPECT_LT(sortText(-10), sortText(-std::numeric_limits<float>::infinity())); + + EXPECT_LT(sortText(1, "z"), sortText(0, "a")); + EXPECT_LT(sortText(0, "a"), sortText(0, "z")); +} + +} // namespace +} // namespace clangd +} // namespace clang Index: unittests/clangd/FileIndexTests.cpp =================================================================== --- unittests/clangd/FileIndexTests.cpp +++ unittests/clangd/FileIndexTests.cpp @@ -7,11 +7,8 @@ // //===----------------------------------------------------------------------===// -#include "TestFS.h" +#include "TestTU.h" #include "index/FileIndex.h" -#include "clang/Frontend/CompilerInvocation.h" -#include "clang/Frontend/PCHContainerOperations.h" -#include "clang/Frontend/Utils.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -83,38 +80,19 @@ return Matches; } -/// Create an ParsedAST for \p Code. Returns None if \p Code is empty. -/// \p Code is put into <Path>.h which is included by \p <BasePath>.cpp. -llvm::Optional<ParsedAST> build(llvm::StringRef BasePath, - llvm::StringRef Code) { - if (Code.empty()) - return llvm::None; - - assert(llvm::sys::path::extension(BasePath).empty() && - "BasePath must be a base file path without extension."); - llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> VFS( - new vfs::InMemoryFileSystem); - std::string Path = testPath((BasePath + ".cpp").str()); - std::string Header = testPath((BasePath + ".h").str()); - VFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("")); - VFS->addFile(Header, 0, llvm::MemoryBuffer::getMemBuffer(Code)); - const char *Args[] = {"clang", "-xc++", "-include", Header.c_str(), - Path.c_str()}; - - auto CI = createInvocationFromCommandLine(Args); - - auto Buf = llvm::MemoryBuffer::getMemBuffer(Code); - auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf), - std::make_shared<PCHContainerOperations>(), VFS); - assert(AST.hasValue()); - return std::move(*AST); +// Adds Basename.cpp, which includes Basename.h, which contains Code. +void update(FileIndex &M, llvm::StringRef Basename, llvm::StringRef Code) { + TestTU File; + File.Filename = (Basename + ".cpp").str(); + File.HeaderFilename = (Basename + ".h").str(); + File.HeaderCode = Code; + auto AST = File.build(); + M.update(File.Filename, &AST); } TEST(FileIndexTest, IndexAST) { FileIndex M; - M.update( - "f1", - build("f1", "namespace ns { void f() {} class X {}; }").getPointer()); + update(M, "f1", "namespace ns { void f() {} class X {}; }"); FuzzyFindRequest Req; Req.Query = ""; @@ -124,24 +102,17 @@ TEST(FileIndexTest, NoLocal) { FileIndex M; - M.update( - "f1", - build("f1", "namespace ns { void f() { int local = 0; } class X {}; }") - .getPointer()); + update(M, "f1", "namespace ns { void f() { int local = 0; } class X {}; }"); FuzzyFindRequest Req; Req.Query = ""; EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns", "ns::f", "ns::X")); } TEST(FileIndexTest, IndexMultiASTAndDeduplicate) { FileIndex M; - M.update( - "f1", - build("f1", "namespace ns { void f() {} class X {}; }").getPointer()); - M.update( - "f2", - build("f2", "namespace ns { void ff() {} class X {}; }").getPointer()); + update(M, "f1", "namespace ns { void f() {} class X {}; }"); + update(M, "f2", "namespace ns { void ff() {} class X {}; }"); FuzzyFindRequest Req; Req.Query = ""; @@ -151,39 +122,35 @@ TEST(FileIndexTest, RemoveAST) { FileIndex M; - M.update( - "f1", - build("f1", "namespace ns { void f() {} class X {}; }").getPointer()); + update(M, "f1", "namespace ns { void f() {} class X {}; }"); FuzzyFindRequest Req; Req.Query = ""; Req.Scopes = {"ns::"}; EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X")); - M.update("f1", nullptr); + M.update("f1.cpp", nullptr); EXPECT_THAT(match(M, Req), UnorderedElementsAre()); } TEST(FileIndexTest, RemoveNonExisting) { FileIndex M; - M.update("no", nullptr); + M.update("no.cpp", nullptr); EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre()); } TEST(FileIndexTest, IgnoreClassMembers) { FileIndex M; - M.update("f1", - build("f1", "class X { static int m1; int m2; static void f(); };") - .getPointer()); + update(M, "f1", "class X { static int m1; int m2; static void f(); };"); FuzzyFindRequest Req; Req.Query = ""; EXPECT_THAT(match(M, Req), UnorderedElementsAre("X")); } TEST(FileIndexTest, NoIncludeCollected) { FileIndex M; - M.update("f", build("f", "class string {};").getPointer()); + update(M, "f", "class string {};"); FuzzyFindRequest Req; Req.Query = ""; @@ -206,7 +173,7 @@ )cpp"; FileIndex M; - M.update("f", build("f", Source).getPointer()); + update(M, "f", Source); FuzzyFindRequest Req; Req.Query = ""; Index: unittests/clangd/DraftStoreTests.cpp =================================================================== --- unittests/clangd/DraftStoreTests.cpp +++ unittests/clangd/DraftStoreTests.cpp @@ -241,7 +241,7 @@ EXPECT_TRUE(!Result); EXPECT_EQ(llvm::toString(Result.takeError()), - "Character value is out of range (100)"); + "UTF-16 offset 100 is invalid for line 0"); } TEST(DraftStoreIncrementalUpdateTest, EndCharOutOfRange) { @@ -262,7 +262,7 @@ EXPECT_TRUE(!Result); EXPECT_EQ(llvm::toString(Result.takeError()), - "Character value is out of range (100)"); + "UTF-16 offset 100 is invalid for line 0"); } TEST(DraftStoreIncrementalUpdateTest, StartLineOutOfRange) { @@ -338,7 +338,7 @@ EXPECT_TRUE(!Result); EXPECT_EQ(llvm::toString(Result.takeError()), - "Character value is out of range (100)"); + "UTF-16 offset 100 is invalid for line 0"); llvm::Optional<std::string> Contents = DS.getDraft(File); EXPECT_TRUE(Contents); Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -144,6 +144,13 @@ Symbol var(StringRef Name) { return sym(Name, index::SymbolKind::Variable, "@\\0"); } +Symbol ns(StringRef Name) { + return sym(Name, index::SymbolKind::Namespace, "@N@\\0"); +} +Symbol withReferences(int N, Symbol S) { + S.References = N; + return S; +} TEST(CompletionTest, Limit) { clangd::CodeCompleteOptions Opts; @@ -443,6 +450,14 @@ UnorderedElementsAre(AllOf(Named("XYZ"), Filter("XYZ")))); } +TEST(CompletionTest, ReferencesAffectRanking) { + auto Results = completions("int main() { abs^ }", {ns("absl"), func("abs")}); + EXPECT_THAT(Results.items, HasSubsequence(Named("abs"), Named("absl"))); + Results = completions("int main() { abs^ }", + {withReferences(10000, ns("absl")), func("abs")}); + EXPECT_THAT(Results.items, HasSubsequence(Named("absl"), Named("abs"))); +} + TEST(CompletionTest, GlobalQualified) { auto Results = completions( R"cpp( Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -9,10 +9,8 @@ #include "Annotations.h" #include "ClangdUnit.h" -#include "TestFS.h" -#include "clang/Frontend/CompilerInvocation.h" -#include "clang/Frontend/PCHContainerOperations.h" -#include "clang/Frontend/Utils.h" +#include "SourceCode.h" +#include "TestTU.h" #include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -35,24 +33,6 @@ return Field(&Diag::Notes, ElementsAre(NoteMatcher)); } -// FIXME: this is duplicated with FileIndexTests. Share it. -ParsedAST build(StringRef Code, std::vector<const char *> Flags = {}) { - std::vector<const char *> Cmd = {"clang", "main.cpp"}; - Cmd.insert(Cmd.begin() + 1, Flags.begin(), Flags.end()); - auto CI = createInvocationFromCommandLine(Cmd); - auto Buf = MemoryBuffer::getMemBuffer(Code); - auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf), - std::make_shared<PCHContainerOperations>(), - vfs::getRealFileSystem()); - assert(AST.hasValue()); - return std::move(*AST); -} - -std::vector<Diag> buildDiags(llvm::StringRef Code, - std::vector<const char *> Flags = {}) { - return build(Code, std::move(Flags)).getDiagnostics(); -} - MATCHER_P2(Diag, Range, Message, "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { return arg.Range == Range && arg.Message == Message; @@ -104,7 +84,7 @@ } )cpp"); EXPECT_THAT( - buildDiags(Test.code()), + TestTU::withCode(Test.code()).build().getDiagnostics(), ElementsAre( // This range spans lines. AllOf(Diag(Test.range("typo"), @@ -122,13 +102,15 @@ TEST(DiagnosticsTest, FlagsMatter) { Annotations Test("[[void]] main() {}"); - EXPECT_THAT(buildDiags(Test.code()), + auto TU = TestTU::withCode(Test.code()); + EXPECT_THAT(TU.build().getDiagnostics(), ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"), WithFix(Fix(Test.range(), "int", "change 'void' to 'int'"))))); // Same code built as C gets different diagnostics. + TU.Filename = "Plain.c"; EXPECT_THAT( - buildDiags(Test.code(), {"-x", "c"}), + TU.build().getDiagnostics(), ElementsAre(AllOf( Diag(Test.range(), "return type of 'main' is not 'int'"), WithFix(Fix(Test.range(), "int", "change return type to 'int'"))))); @@ -149,7 +131,7 @@ #endif )cpp"); EXPECT_THAT( - buildDiags(Test.code()), + TestTU::withCode(Test.code()).build().getDiagnostics(), ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); } @@ -216,6 +198,28 @@ Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty()))); } +TEST(ClangdUnitTest, GetBeginningOfIdentifier) { + // First ^ is the expected beginning, last is the search position. + for (const char *Text : { + "int ^f^oo();", // inside identifier + "int ^foo();", // beginning of identifier + "int ^foo^();", // end of identifier + "int foo(^);", // non-identifier + "^int foo();", // beginning of file (can't back up) + "int ^f0^0();", // after a digit (lexing at N-1 is wrong) + "int ^λλ^λ();", // UTF-8 handled properly when backing up + }) { + Annotations TestCase(Text); + auto AST = TestTU::withCode(TestCase.code()).build(); + const auto &SourceMgr = AST.getASTContext().getSourceManager(); + SourceLocation Actual = getBeginningOfIdentifier( + AST, TestCase.points().back(), SourceMgr.getMainFileID()); + Position ActualPos = + offsetToPosition(TestCase.code(), SourceMgr.getFileOffset(Actual)); + EXPECT_EQ(TestCase.points().front(), ActualPos) << Text; + } +} + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/CMakeLists.txt =================================================================== --- unittests/clangd/CMakeLists.txt +++ unittests/clangd/CMakeLists.txt @@ -23,10 +23,12 @@ HeadersTests.cpp IndexTests.cpp JSONExprTests.cpp + QualityTests.cpp SourceCodeTests.cpp SymbolCollectorTests.cpp SyncAPI.cpp TestFS.cpp + TestTU.cpp ThreadingTests.cpp TraceTests.cpp TUSchedulerTests.cpp Index: unittests/clang-tidy/ClangTidyOptionsTest.cpp =================================================================== --- unittests/clang-tidy/ClangTidyOptionsTest.cpp +++ unittests/clang-tidy/ClangTidyOptionsTest.cpp @@ -58,38 +58,33 @@ llvm::ErrorOr<ClangTidyOptions> Options = parseConfiguration("Checks: \"-*,misc-*\"\n" "HeaderFilterRegex: \".*\"\n" - "AnalyzeTemporaryDtors: true\n" "User: some.user"); EXPECT_TRUE(!!Options); EXPECT_EQ("-*,misc-*", *Options->Checks); EXPECT_EQ(".*", *Options->HeaderFilterRegex); - EXPECT_TRUE(*Options->AnalyzeTemporaryDtors); EXPECT_EQ("some.user", *Options->User); } TEST(ParseConfiguration, MergeConfigurations) { llvm::ErrorOr<ClangTidyOptions> Options1 = parseConfiguration(R"( Checks: "check1,check2" HeaderFilterRegex: "filter1" - AnalyzeTemporaryDtors: true User: user1 ExtraArgs: ['arg1', 'arg2'] ExtraArgsBefore: ['arg-before1', 'arg-before2'] )"); ASSERT_TRUE(!!Options1); llvm::ErrorOr<ClangTidyOptions> Options2 = parseConfiguration(R"( Checks: "check3,check4" HeaderFilterRegex: "filter2" - AnalyzeTemporaryDtors: false User: user2 ExtraArgs: ['arg3', 'arg4'] ExtraArgsBefore: ['arg-before3', 'arg-before4'] )"); ASSERT_TRUE(!!Options2); ClangTidyOptions Options = Options1->mergeWith(*Options2); EXPECT_EQ("check1,check2,check3,check4", *Options.Checks); EXPECT_EQ("filter2", *Options.HeaderFilterRegex); - EXPECT_FALSE(*Options.AnalyzeTemporaryDtors); EXPECT_EQ("user2", *Options.User); ASSERT_TRUE(Options.ExtraArgs.hasValue()); EXPECT_EQ("arg1,arg2,arg3,arg4", llvm::join(Options.ExtraArgs->begin(), Index: test/clangd/rename.test =================================================================== --- test/clangd/rename.test +++ test/clangd/rename.test @@ -36,4 +36,4 @@ --- {"jsonrpc":"2.0","id":3,"method":"shutdown"} --- -{"jsonrpc":"2.0":"method":"exit"} +{"jsonrpc":"2.0","method":"exit"} Index: test/clang-tidy/temporaries.cpp =================================================================== --- test/clang-tidy/temporaries.cpp +++ test/clang-tidy/temporaries.cpp @@ -1,4 +1,4 @@ -// RUN: clang-tidy -checks='-*,clang-analyzer-core.NullDereference' -analyze-temporary-dtors %s -- | FileCheck %s +// RUN: clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- | FileCheck %s struct NoReturnDtor { ~NoReturnDtor() __attribute__((noreturn)); @@ -17,7 +17,8 @@ void testNullPointerDereference() { int *value = 0; if (check(NoReturnDtor())) { - // This unreachable code causes a warning if we don't run with -analyze-temporary-dtors + // This unreachable code causes a warning if analysis of temporary + // destructors is not enabled. *value = 1; } } Index: test/clang-tidy/objc-property-declaration.m =================================================================== --- test/clang-tidy/objc-property-declaration.m +++ test/clang-tidy/objc-property-declaration.m @@ -19,6 +19,8 @@ @property(strong, nonatomic) NSString *VCsPluralToAdd; @property(assign, nonatomic) int centerX; @property(assign, nonatomic) int enable2GBackgroundFetch; +@property(assign, nonatomic) int shouldUseCFPreferences; +@property(assign, nonatomic) int enableGLAcceleration; @end @interface Foo (Bar) Index: test/clang-tidy/modernize-raw-string-literal.cpp =================================================================== --- test/clang-tidy/modernize-raw-string-literal.cpp +++ test/clang-tidy/modernize-raw-string-literal.cpp @@ -40,6 +40,8 @@ char const *const Us("goink\\\037"); char const *const HexNonPrintable("\\\x03"); char const *const Delete("\\\177"); +char const *const MultibyteSnowman("\xE2\x98\x83"); +// CHECK-FIXES: {{^}}char const *const MultibyteSnowman("\xE2\x98\x83");{{$}} char const *const TrailingSpace("A line \\with space. \n"); char const *const TrailingNewLine("A single \\line.\n"); Index: test/clang-tidy/google-runtime-int.cpp =================================================================== --- test/clang-tidy/google-runtime-int.cpp +++ test/clang-tidy/google-runtime-int.cpp @@ -72,3 +72,20 @@ B a, b; a = b; } + +__attribute__((__format__ (__printf__, 1, 2))) +void myprintf(const char* s, ...); + +void doprint_no_warning() { + uint64 foo = 23; + myprintf("foo %lu %lu", (unsigned long)42, (unsigned long)foo); +} + +void myprintf_no_attribute(const char* s, ...); + +void doprint_warning() { + uint64 foo = 23; + myprintf_no_attribute("foo %lu %lu", (unsigned long)42, (unsigned long)foo); +// CHECK-MESSAGES: [[@LINE-1]]:41: warning: consider replacing 'unsigned long' +// CHECK-MESSAGES: [[@LINE-2]]:60: warning: consider replacing 'unsigned long' +} Index: docs/clang-tidy/index.rst =================================================================== --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -112,11 +112,6 @@ clang-tidy options: - -analyze-temporary-dtors - - Enable temporary destructor-aware analysis in - clang-analyzer- checks. - This option overrides the value read from a - .clang-tidy file. -checks=<string> - Comma-separated list of globs with optional '-' prefix. Globs are processed in order of @@ -245,7 +240,6 @@ Checks: '-*,some-check' WarningsAsErrors: '' HeaderFilterRegex: '' - AnalyzeTemporaryDtors: false FormatStyle: none User: user CheckOptions: Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -162,6 +162,9 @@ Flags functions that have more than a specified number of variables declared in the body. +- The `AnalyzeTemporaryDtors` option was removed, since the corresponding + `cfg-temporary-dtors` option of the Static Analyzer now defaults to `true`. + - New alias :doc:`hicpp-avoid-goto <clang-tidy/checks/hicpp-avoid-goto>` to :doc:`cppcoreguidelines-avoid-goto <clang-tidy/checks/cppcoreguidelines-avoid-goto>` Index: clangd/index/SymbolCollector.cpp =================================================================== --- clangd/index/SymbolCollector.cpp +++ clangd/index/SymbolCollector.cpp @@ -195,14 +195,10 @@ auto TokenLength = clang::Lexer::MeasureTokenLength(NameLoc, SM, LangOpts); auto CreatePosition = [&SM](SourceLocation Loc) { - auto FileIdAndOffset = SM.getDecomposedLoc(Loc); - auto FileId = FileIdAndOffset.first; - auto Offset = FileIdAndOffset.second; + auto LSPLoc = sourceLocToPosition(SM, Loc); SymbolLocation::Position Pos; - // Position is 0-based while SourceManager is 1-based. - Pos.Line = SM.getLineNumber(FileId, Offset) - 1; - // FIXME: Use UTF-16 code units, not UTF-8 bytes. - Pos.Column = SM.getColumnNumber(FileId, Offset) - 1; + Pos.Line = LSPLoc.line; + Pos.Column = LSPLoc.character; return Pos; }; Index: clangd/index/MemIndex.cpp =================================================================== --- clangd/index/MemIndex.cpp +++ clangd/index/MemIndex.cpp @@ -47,7 +47,7 @@ continue; if (auto Score = Filter.match(Sym->Name)) { - Top.emplace(-*Score, Sym); + Top.emplace(-*Score * quality(*Sym), Sym); if (Top.size() > Req.MaxCandidateCount) { More = true; Top.pop(); Index: clangd/index/Index.h =================================================================== --- clangd/index/Index.h +++ clangd/index/Index.h @@ -199,6 +199,12 @@ }; llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S); +// Computes query-independent quality score for a Symbol. +// This currently falls in the range [1, ln(#indexed documents)]. +// FIXME: this should probably be split into symbol -> signals +// and signals -> score, so it can be reused for Sema completions. +double quality(const Symbol &S); + // An immutable symbol container that stores a set of symbols. // The container will maintain the lifetime of the symbols. class SymbolSlab { Index: clangd/index/Index.cpp =================================================================== --- clangd/index/Index.cpp +++ clangd/index/Index.cpp @@ -48,6 +48,14 @@ return OS << S.Scope << S.Name; } +double quality(const Symbol &S) { + // This avoids a sharp gradient for tail symbols, and also neatly avoids the + // question of whether 0 references means a bad symbol or missing data. + if (S.References < 3) + return 1; + return std::log(S.References); +} + 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) { Index: clangd/index/FileIndex.h =================================================================== --- clangd/index/FileIndex.h +++ clangd/index/FileIndex.h @@ -71,6 +71,10 @@ MemIndex Index; }; +/// Retrieves namespace and class level symbols in \p AST. +/// Exposed to assist in unit tests. +SymbolSlab indexAST(ParsedAST *AST); + } // namespace clangd } // namespace clang Index: clangd/index/FileIndex.cpp =================================================================== --- clangd/index/FileIndex.cpp +++ clangd/index/FileIndex.cpp @@ -13,12 +13,9 @@ namespace clang { namespace clangd { -namespace { -/// Retrieves namespace and class level symbols in \p Decls. -std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx, - std::shared_ptr<Preprocessor> PP, - llvm::ArrayRef<const Decl *> Decls) { +SymbolSlab indexAST(ParsedAST *AST) { + assert(AST && "AST must not be nullptr!"); SymbolCollector::Options CollectorOpts; // FIXME(ioeric): we might also want to collect include headers. We would need // to make sure all includes are canonicalized (with CanonicalIncludes), which @@ -29,21 +26,18 @@ CollectorOpts.CountReferences = false; SymbolCollector Collector(std::move(CollectorOpts)); - Collector.setPreprocessor(std::move(PP)); + Collector.setPreprocessor(AST->getPreprocessorPtr()); index::IndexingOptions IndexOpts; // We only need declarations, because we don't count references. IndexOpts.SystemSymbolFilter = index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly; IndexOpts.IndexFunctionLocals = false; - index::indexTopLevelDecls(Ctx, Decls, Collector, IndexOpts); - auto Symbols = llvm::make_unique<SymbolSlab>(); - *Symbols = Collector.takeSymbols(); - return Symbols; + index::indexTopLevelDecls(AST->getASTContext(), AST->getTopLevelDecls(), + Collector, IndexOpts); + return Collector.takeSymbols(); } -} // namespace - void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Slab) { std::lock_guard<std::mutex> Lock(Mutex); if (!Slab) @@ -79,8 +73,8 @@ if (!AST) { FSymbols.update(Path, nullptr); } else { - auto Slab = indexAST(AST->getASTContext(), AST->getPreprocessorPtr(), - AST->getTopLevelDecls()); + auto Slab = llvm::make_unique<SymbolSlab>(); + *Slab = indexAST(AST); FSymbols.update(Path, std::move(Slab)); } auto Symbols = FSymbols.allSymbols(); Index: clangd/XRefs.h =================================================================== --- clangd/XRefs.h +++ clangd/XRefs.h @@ -15,13 +15,15 @@ #include "ClangdUnit.h" #include "Protocol.h" +#include "index/Index.h" #include <vector> namespace clang { namespace clangd { /// Get definition of symbol at a specified \p Pos. -std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos); +std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos, + const SymbolIndex *Index = nullptr); /// Returns highlights for all usages of a symbol at \p Pos. std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST, Index: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -14,6 +14,7 @@ #include "clang/AST/DeclTemplate.h" #include "clang/Index/IndexDataConsumer.h" #include "clang/Index/IndexingAction.h" +#include "clang/Index/USRGeneration.h" #include "llvm/Support/Path.h" namespace clang { namespace clangd { @@ -34,6 +35,33 @@ return nullptr; } +// Convert a SymbolLocation to LSP's Location. +// HintPath is used to resolve the path of URI. +// FIXME: figure out a good home for it, and share the implementation with +// FindSymbols. +llvm::Optional<Location> ToLSPLocation(const SymbolLocation &Loc, + llvm::StringRef HintPath) { + if (!Loc) + return llvm::None; + auto Uri = URI::parse(Loc.FileURI); + if (!Uri) { + log("Could not parse URI: " + Loc.FileURI); + return llvm::None; + } + auto Path = URI::resolve(*Uri, HintPath); + if (!Path) { + log("Could not resolve URI: " + Loc.FileURI); + return llvm::None; + } + Location LSPLoc; + LSPLoc.uri = URIForFile(*Path); + LSPLoc.range.start.line = Loc.Start.Line; + LSPLoc.range.start.character = Loc.Start.Column; + LSPLoc.range.end.line = Loc.End.Line; + LSPLoc.range.end.character = Loc.End.Column; + return LSPLoc; +} + struct MacroDecl { StringRef Name; const MacroInfo *Info; @@ -128,6 +156,38 @@ } }; +struct IdentifiedSymbol { + std::vector<const Decl *> Decls; + std::vector<MacroDecl> Macros; +}; + +IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) { + auto DeclMacrosFinder = DeclarationAndMacrosFinder( + llvm::errs(), Pos, AST.getASTContext(), AST.getPreprocessor()); + index::IndexingOptions IndexOpts; + IndexOpts.SystemSymbolFilter = + index::IndexingOptions::SystemSymbolFilterKind::All; + IndexOpts.IndexFunctionLocals = true; + indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(), + DeclMacrosFinder, IndexOpts); + + return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()}; +} + +llvm::Optional<std::string> +getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr) { + SmallString<64> FilePath = F->tryGetRealPathName(); + if (FilePath.empty()) + FilePath = F->getName(); + if (!llvm::sys::path::is_absolute(FilePath)) { + if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) { + log("Could not turn relative path to absolute: " + FilePath); + return llvm::None; + } + } + return FilePath.str().str(); +} + llvm::Optional<Location> makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); @@ -145,30 +205,32 @@ Range R = {Begin, End}; Location L; - SmallString<64> FilePath = F->tryGetRealPathName(); - if (FilePath.empty()) - FilePath = F->getName(); - if (!llvm::sys::path::is_absolute(FilePath)) { - if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) { - log("Could not turn relative path to absolute: " + FilePath); - return llvm::None; - } + auto FilePath = getAbsoluteFilePath(F, SourceMgr); + if (!FilePath) { + log("failed to get path!"); + return llvm::None; } - - L.uri = URIForFile(FilePath.str()); + L.uri = URIForFile(*FilePath); L.range = R; return L; } +// Get the symbol ID for a declaration, if possible. +llvm::Optional<SymbolID> getSymbolID(const Decl *D) { + llvm::SmallString<128> USR; + if (index::generateUSRForDecl(D, USR)) { + return None; + } + return SymbolID(USR); +} + } // namespace -std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos) { +std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos, + const SymbolIndex *Index) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); - const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); - if (!FE) - return {}; - - SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE); + SourceLocation SourceLocationBeg = + getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID()); std::vector<Location> Result; // Handle goto definition for #include. @@ -182,32 +244,97 @@ if (!Result.empty()) return Result; - DeclarationAndMacrosFinder DeclMacrosFinder(llvm::errs(), SourceLocationBeg, - AST.getASTContext(), - AST.getPreprocessor()); - index::IndexingOptions IndexOpts; - IndexOpts.SystemSymbolFilter = - index::IndexingOptions::SystemSymbolFilterKind::All; - IndexOpts.IndexFunctionLocals = true; - - indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(), - DeclMacrosFinder, IndexOpts); - - std::vector<const Decl *> Decls = DeclMacrosFinder.takeDecls(); - std::vector<MacroDecl> MacroInfos = DeclMacrosFinder.takeMacroInfos(); + // Identified symbols at a specific position. + auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg); - for (auto D : Decls) { - auto Loc = findNameLoc(D); + for (auto Item : Symbols.Macros) { + auto Loc = Item.Info->getDefinitionLoc(); auto L = makeLocation(AST, SourceRange(Loc, Loc)); if (L) Result.push_back(*L); } - for (auto Item : MacroInfos) { - auto Loc = Item.Info->getDefinitionLoc(); + // Declaration and definition are different terms in C-family languages, and + // LSP only defines the "GoToDefinition" specification, so we try to perform + // the "most sensible" GoTo operation: + // + // - We use the location from AST and index (if available) to provide the + // final results. When there are duplicate results, we prefer AST over + // index because AST is more up-to-date. + // + // - For each symbol, we will return a location of the canonical declaration + // (e.g. function declaration in header), and a location of definition if + // they are available. + // + // So the work flow: + // + // 1. Identify the symbols being search for by traversing the AST. + // 2. Populate one of the locations with the AST location. + // 3. Use the AST information to query the index, and populate the index + // location (if available). + // 4. Return all populated locations for all symbols, definition first ( + // which we think is the users wants most often). + struct CandidateLocation { + llvm::Optional<Location> Def; + llvm::Optional<Location> Decl; + }; + llvm::DenseMap<SymbolID, CandidateLocation> ResultCandidates; + + // Emit all symbol locations (declaration or definition) from AST. + for (const auto *D : Symbols.Decls) { + // Fake key for symbols don't have USR (no SymbolID). + // Ideally, there should be a USR for each identified symbols. Symbols + // without USR are rare and unimportant cases, we use the a fake holder to + // minimize the invasiveness of these cases. + SymbolID Key(""); + if (auto ID = getSymbolID(D)) + Key = *ID; + + auto &Candidate = ResultCandidates[Key]; + auto Loc = findNameLoc(D); auto L = makeLocation(AST, SourceRange(Loc, Loc)); - if (L) - Result.push_back(*L); + // The declaration in the identified symbols is a definition if possible + // otherwise it is declaration. + bool IsDef = GetDefinition(D) == D; + // Populate one of the slots with location for the AST. + if (!IsDef) + Candidate.Decl = L; + else + Candidate.Def = L; + } + + if (Index) { + LookupRequest QueryRequest; + // Build request for index query, using SymbolID. + for (auto It : ResultCandidates) + QueryRequest.IDs.insert(It.first); + std::string HintPath; + const FileEntry *FE = + SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); + if (auto Path = getAbsoluteFilePath(FE, SourceMgr)) + HintPath = *Path; + // Query the index and populate the empty slot. + Index->lookup( + QueryRequest, [&HintPath, &ResultCandidates](const Symbol &Sym) { + auto It = ResultCandidates.find(Sym.ID); + assert(It != ResultCandidates.end()); + auto &Value = It->second; + + if (!Value.Def) + Value.Def = ToLSPLocation(Sym.Definition, HintPath); + if (!Value.Decl) + Value.Decl = ToLSPLocation(Sym.CanonicalDeclaration, HintPath); + }); + } + + // Populate the results, definition first. + for (auto It : ResultCandidates) { + const auto &Candidate = It.second; + if (Candidate.Def) + Result.push_back(*Candidate.Def); + if (Candidate.Decl && + Candidate.Decl != Candidate.Def) // Decl and Def might be the same + Result.push_back(*Candidate.Decl); } return Result; @@ -280,29 +407,19 @@ std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST, Position Pos) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); - const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); - if (!FE) - return {}; + SourceLocation SourceLocationBeg = + getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID()); - SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE); + auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg); + std::vector<const Decl *> SelectedDecls = Symbols.Decls; + + DocumentHighlightsFinder DocHighlightsFinder( + llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls); - DeclarationAndMacrosFinder DeclMacrosFinder(llvm::errs(), SourceLocationBeg, - AST.getASTContext(), - AST.getPreprocessor()); index::IndexingOptions IndexOpts; IndexOpts.SystemSymbolFilter = index::IndexingOptions::SystemSymbolFilterKind::All; IndexOpts.IndexFunctionLocals = true; - - // Macro occurences are not currently handled. - indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(), - DeclMacrosFinder, IndexOpts); - - std::vector<const Decl *> SelectedDecls = DeclMacrosFinder.takeDecls(); - - DocumentHighlightsFinder DocHighlightsFinder( - llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls); - indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(), DocHighlightsFinder, IndexOpts); @@ -413,30 +530,16 @@ Hover getHover(ParsedAST &AST, Position Pos) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); - const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); - if (FE == nullptr) - return Hover(); - - SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE); - DeclarationAndMacrosFinder DeclMacrosFinder(llvm::errs(), SourceLocationBeg, - AST.getASTContext(), - AST.getPreprocessor()); - - index::IndexingOptions IndexOpts; - IndexOpts.SystemSymbolFilter = - index::IndexingOptions::SystemSymbolFilterKind::All; - IndexOpts.IndexFunctionLocals = true; - - indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(), - DeclMacrosFinder, IndexOpts); + SourceLocation SourceLocationBeg = + getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID()); + // Identified symbols at a specific position. + auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg); - std::vector<MacroDecl> Macros = DeclMacrosFinder.takeMacroInfos(); - if (!Macros.empty()) - return getHoverContents(Macros[0].Name); + if (!Symbols.Macros.empty()) + return getHoverContents(Symbols.Macros[0].Name); - std::vector<const Decl *> Decls = DeclMacrosFinder.takeDecls(); - if (!Decls.empty()) - return getHoverContents(Decls[0]); + if (!Symbols.Decls.empty()) + return getHoverContents(Symbols.Decls[0]); return Hover(); } Index: clangd/SourceCode.h =================================================================== --- clangd/SourceCode.h +++ clangd/SourceCode.h @@ -23,22 +23,17 @@ /// Turn a [line, column] pair into an offset in Code. /// -/// If the character value is greater than the line length, the behavior depends -/// on AllowColumnsBeyondLineLength: -/// -/// - if true: default back to the end of the line -/// - if false: return an error -/// -/// If the line number is greater than the number of lines in the document, -/// always return an error. +/// If P.character exceeds the line length, returns the offset at end-of-line. +/// (If !AllowColumnsBeyondLineLength, then returns an error instead). +/// If the line number is out of range, returns an error. /// /// The returned value is in the range [0, Code.size()]. llvm::Expected<size_t> positionToOffset(llvm::StringRef Code, Position P, bool AllowColumnsBeyondLineLength = true); /// Turn an offset in Code into a [line, column] pair. -/// FIXME: This should return an error if the offset is invalid. +/// The offset must be in range [0, Code.size()]. Position offsetToPosition(llvm::StringRef Code, size_t Offset); /// Turn a SourceLocation into a [line, column] pair. @@ -49,6 +44,12 @@ // Note that clang also uses closed source ranges, which this can't handle! Range halfOpenToRange(const SourceManager &SM, CharSourceRange R); +// Converts an offset to a clang line/column (1-based, columns are bytes). +// The offset must be in range [0, Code.size()]. +// Prefer to use SourceManager if one is available. +std::pair<size_t, size_t> offsetToClangLineColumn(llvm::StringRef Code, + size_t Offset); + /// From "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no /// qualifier. std::pair<llvm::StringRef, llvm::StringRef> Index: clangd/SourceCode.cpp =================================================================== --- clangd/SourceCode.cpp +++ clangd/SourceCode.cpp @@ -16,6 +16,66 @@ namespace clangd { using namespace llvm; +// Here be dragons. LSP positions use columns measured in *UTF-16 code units*! +// Clangd uses UTF-8 and byte-offsets internally, so conversion is nontrivial. + +// Iterates over unicode codepoints in the (UTF-8) string. For each, +// invokes CB(UTF-8 length, UTF-16 length), and breaks if it returns true. +// Returns true if CB returned true, false if we hit the end of string. +template <typename Callback> +static bool iterateCodepoints(StringRef U8, const Callback &CB) { + for (size_t I = 0; I < U8.size();) { + unsigned char C = static_cast<unsigned char>(U8[I]); + if (LLVM_LIKELY(!(C & 0x80))) { // ASCII character. + if (CB(1, 1)) + return true; + ++I; + continue; + } + // This convenient property of UTF-8 holds for all non-ASCII characters. + size_t UTF8Length = countLeadingOnes(C); + // 0xxx is ASCII, handled above. 10xxx is a trailing byte, invalid here. + // 11111xxx is not valid UTF-8 at all. Assert because it's probably our bug. + assert((UTF8Length >= 2 && UTF8Length <= 4) && + "Invalid UTF-8, or transcoding bug?"); + I += UTF8Length; // Skip over all trailing bytes. + // A codepoint takes two UTF-16 code unit if it's astral (outside BMP). + // Astral codepoints are encoded as 4 bytes in UTF-8 (11110xxx ...) + if (CB(UTF8Length, UTF8Length == 4 ? 2 : 1)) + return true; + } + return false; +} + +// Returns the offset into the string that matches \p Units UTF-16 code units. +// Conceptually, this converts to UTF-16, truncates to CodeUnits, converts back +// to UTF-8, and returns the length in bytes. +static size_t measureUTF16(StringRef U8, int U16Units, bool &Valid) { + size_t Result = 0; + Valid = U16Units == 0 || iterateCodepoints(U8, [&](int U8Len, int U16Len) { + Result += U8Len; + U16Units -= U16Len; + return U16Units <= 0; + }); + if (U16Units < 0) // Offset was into the middle of a surrogate pair. + Valid = false; + // Don't return an out-of-range index if we overran. + return std::min(Result, U8.size()); +} + +// Counts the number of UTF-16 code units needed to represent a string. +// Like most strings in clangd, the input is UTF-8 encoded. +static size_t utf16Len(StringRef U8) { + // A codepoint takes two UTF-16 code unit if it's astral (outside BMP). + // Astral codepoints are encoded as 4 bytes in UTF-8, starting with 11110xxx. + size_t Count = 0; + iterateCodepoints(U8, [&](int U8Len, int U16Len) { + Count += U16Len; + return false; + }); + return Count; +} + llvm::Expected<size_t> positionToOffset(StringRef Code, Position P, bool AllowColumnsBeyondLineLength) { if (P.line < 0) @@ -40,31 +100,43 @@ if (NextNL == StringRef::npos) NextNL = Code.size(); - if (StartOfLine + P.character > NextNL && !AllowColumnsBeyondLineLength) + bool Valid; + size_t ByteOffsetInLine = measureUTF16( + Code.substr(StartOfLine, NextNL - StartOfLine), P.character, Valid); + if (!Valid && !AllowColumnsBeyondLineLength) return llvm::make_error<llvm::StringError>( - llvm::formatv("Character value is out of range ({0})", P.character), + llvm::formatv("UTF-16 offset {0} is invalid for line {1}", P.character, + P.line), llvm::errc::invalid_argument); - // FIXME: officially P.character counts UTF-16 code units, not UTF-8 bytes! - return std::min(NextNL, StartOfLine + P.character); + return StartOfLine + ByteOffsetInLine; } Position offsetToPosition(StringRef Code, size_t Offset) { Offset = std::min(Code.size(), Offset); StringRef Before = Code.substr(0, Offset); int Lines = Before.count('\n'); size_t PrevNL = Before.rfind('\n'); size_t StartOfLine = (PrevNL == StringRef::npos) ? 0 : (PrevNL + 1); - // FIXME: officially character counts UTF-16 code units, not UTF-8 bytes! Position Pos; Pos.line = Lines; - Pos.character = static_cast<int>(Offset - StartOfLine); + Pos.character = utf16Len(Before.substr(StartOfLine)); return Pos; } Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) { + // We use the SourceManager's line tables, but its column number is in bytes. + FileID FID; + unsigned Offset; + std::tie(FID, Offset) = SM.getDecomposedSpellingLoc(Loc); Position P; - P.line = static_cast<int>(SM.getSpellingLineNumber(Loc)) - 1; - P.character = static_cast<int>(SM.getSpellingColumnNumber(Loc)) - 1; + P.line = static_cast<int>(SM.getLineNumber(FID, Offset)) - 1; + bool Invalid = false; + StringRef Code = SM.getBufferData(FID, &Invalid); + if (!Invalid) { + auto ColumnInBytes = SM.getColumnNumber(FID, Offset) - 1; + auto LineSoFar = Code.substr(Offset - ColumnInBytes, ColumnInBytes); + P.character = utf16Len(LineSoFar); + } return P; } @@ -76,6 +148,16 @@ return {Begin, End}; } +std::pair<size_t, size_t> offsetToClangLineColumn(StringRef Code, + size_t Offset) { + Offset = std::min(Code.size(), Offset); + StringRef Before = Code.substr(0, Offset); + int Lines = Before.count('\n'); + size_t PrevNL = Before.rfind('\n'); + size_t StartOfLine = (PrevNL == StringRef::npos) ? 0 : (PrevNL + 1); + return {Lines + 1, Offset - StartOfLine + 1}; +} + std::pair<llvm::StringRef, llvm::StringRef> splitQualifiedName(llvm::StringRef QName) { size_t Pos = QName.rfind("::"); Index: clangd/Quality.h =================================================================== --- /dev/null +++ clangd/Quality.h @@ -0,0 +1,126 @@ +//===--- Quality.h - Ranking alternatives for ambiguous queries -*- C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// +/// +/// Some operations such as code completion produce a set of candidates. +/// Usually the user can choose between them, but we should put the best options +/// at the top (they're easier to select, and more likely to be seen). +/// +/// This file defines building blocks for ranking candidates. +/// It's used by the features directly and also in the implementation of +/// indexes, as indexes also need to heuristically limit their results. +/// +/// The facilities here are: +/// - retrieving scoring signals from e.g. indexes, AST, CodeCompletionString +/// These are structured in a way that they can be debugged, and are fairly +/// consistent regardless of the source. +/// - compute scores from scoring signals. These are suitable for sorting. +/// - sorting utilities like the TopN container. +/// These could be split up further to isolate dependencies if we care. +/// +//===---------------------------------------------------------------------===// +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H +#include "llvm/ADT/StringRef.h" +#include <algorithm> +#include <functional> +#include <vector> +namespace llvm { +class raw_ostream; +} +namespace clang { +class CodeCompletionResult; +namespace clangd { +struct Symbol; + +// Signals structs are designed to be aggregated from 0 or more sources. +// A default instance has neutral signals, and sources are merged into it. +// They can be dumped for debugging, and evaluate()d into a score. + +// Attributes of a symbol that affect how much we like it. +struct SymbolQualitySignals { + 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 Deprecated = false; + unsigned References = 0; + + void merge(const CodeCompletionResult &SemaCCResult); + void merge(const Symbol &IndexResult); + + // Condense these signals down to a single number, higher is better. + float evaluate() const; +}; +llvm::raw_ostream &operator<<(llvm::raw_ostream &, + const SymbolQualitySignals &); + +// Attributes of a symbol-query pair that affect how much we like it. +struct SymbolRelevanceSignals { + // 0-1 fuzzy-match score for unqualified name. Must be explicitly assigned. + float NameMatch = 1; + bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private). + + void merge(const CodeCompletionResult &SemaResult); + + // Condense these signals down to a single number, higher is better. + float evaluate() const; +}; +llvm::raw_ostream &operator<<(llvm::raw_ostream &, + const SymbolRelevanceSignals &); + +// Combine symbol quality and relevance into a single score. +float evaluateSymbolAndRelevance(float SymbolQuality, float SymbolRelevance); + +// TopN<T> is a lossy container that preserves only the "best" N elements. +template <typename T, typename Compare = std::greater<T>> class TopN { +public: + using value_type = T; + TopN(size_t N, Compare Greater = Compare()) + : N(N), Greater(std::move(Greater)) {} + + // Adds a candidate to the set. + // Returns true if a candidate was dropped to get back under N. + bool push(value_type &&V) { + bool Dropped = false; + if (Heap.size() >= N) { + Dropped = true; + if (N > 0 && Greater(V, Heap.front())) { + std::pop_heap(Heap.begin(), Heap.end(), Greater); + Heap.back() = std::move(V); + std::push_heap(Heap.begin(), Heap.end(), Greater); + } + } else { + Heap.push_back(std::move(V)); + std::push_heap(Heap.begin(), Heap.end(), Greater); + } + assert(Heap.size() <= N); + assert(std::is_heap(Heap.begin(), Heap.end(), Greater)); + return Dropped; + } + + // Returns candidates from best to worst. + std::vector<value_type> items() && { + std::sort_heap(Heap.begin(), Heap.end(), Greater); + assert(Heap.size() <= N); + return std::move(Heap); + } + +private: + const size_t N; + std::vector<value_type> Heap; // Min-heap, comparator is Greater. + Compare Greater; +}; + +// Returns a string that sorts in the same order as (-Score, Tiebreak), for LSP. +// (The highest score compares smallest so it sorts at the top). +std::string sortText(float Score, llvm::StringRef Tiebreak = ""); + +} // namespace clangd +} // namespace clang + +#endif Index: clangd/Quality.cpp =================================================================== --- /dev/null +++ clangd/Quality.cpp @@ -0,0 +1,108 @@ +//===--- Quality.cpp --------------------------------------------*- C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// +#include "Quality.h" +#include "index/Index.h" +#include "clang/Sema/CodeCompleteConsumer.h" +#include "llvm/Support/FormatVariadic.h" +#include "llvm/Support/MathExtras.h" +#include "llvm/Support/raw_ostream.h" + +namespace clang { +namespace clangd { +using namespace llvm; + +void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) { + SemaCCPriority = SemaCCResult.Priority; + + if (SemaCCResult.Availability == CXAvailability_Deprecated) + Deprecated = true; +} + +void SymbolQualitySignals::merge(const Symbol &IndexResult) { + References = std::max(IndexResult.References, References); +} + +float SymbolQualitySignals::evaluate() const { + float Score = 1; + + // This avoids a sharp gradient for tail symbols, and also neatly avoids the + // question of whether 0 references means a bad symbol or missing data. + if (References >= 3) + Score *= std::log(References); + + if (SemaCCPriority) + // Map onto a 0-2 interval, so we don't reward/penalize non-Sema results. + // Priority 80 is a really bad score. + Score *= 2 - std::min<float>(80, SemaCCPriority) / 40; + + if (Deprecated) + Score *= 0.1; + + return Score; +} + +raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) { + OS << formatv("=== Symbol quality: {0}\n", S.evaluate()); + if (S.SemaCCPriority) + OS << formatv("\tSemaCCPriority: {0}\n", S.SemaCCPriority); + OS << formatv("\tReferences: {0}\n", S.References); + OS << formatv("\tDeprecated: {0}\n", S.Deprecated); + return OS; +} + +void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) { + if (SemaCCResult.Availability == CXAvailability_NotAvailable || + SemaCCResult.Availability == CXAvailability_NotAccessible) + Forbidden = true; +} + +float SymbolRelevanceSignals::evaluate() const { + if (Forbidden) + return 0; + return NameMatch; +} +raw_ostream &operator<<(raw_ostream &OS, const SymbolRelevanceSignals &S) { + OS << formatv("=== Symbol relevance: {0}\n", S.evaluate()); + OS << formatv("\tName match: {0}\n", S.NameMatch); + OS << formatv("\tForbidden: {0}\n", S.Forbidden); + return OS; +} + +float evaluateSymbolAndRelevance(float SymbolQuality, float SymbolRelevance) { + return SymbolQuality * SymbolRelevance; +} + +// Produces an integer that sorts in the same order as F. +// That is: a < b <==> encodeFloat(a) < encodeFloat(b). +static uint32_t encodeFloat(float F) { + static_assert(std::numeric_limits<float>::is_iec559, ""); + constexpr uint32_t TopBit = ~(~uint32_t{0} >> 1); + + // Get the bits of the float. Endianness is the same as for integers. + uint32_t U = FloatToBits(F); + // IEEE 754 floats compare like sign-magnitude integers. + if (U & TopBit) // Negative float. + return 0 - U; // Map onto the low half of integers, order reversed. + return U + TopBit; // Positive floats map onto the high half of integers. +} + +std::string sortText(float Score, llvm::StringRef Name) { + // We convert -Score to an integer, and hex-encode for readability. + // Example: [0.5, "foo"] -> "41000000foo" + std::string S; + llvm::raw_string_ostream OS(S); + write_hex(OS, encodeFloat(-Score), llvm::HexPrintStyle::Lower, + /*Width=*/2 * sizeof(Score)); + OS << Name; + OS.flush(); + return S; +} + +} // namespace clangd +} // namespace clang Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -91,6 +91,8 @@ int line = 0; /// Character offset on a line in a document (zero-based). + /// WARNING: this is in UTF-16 codepoints, not bytes or characters! + /// Use the functions in SourceCode.h to construct/interpret Positions. int character = 0; friend bool operator==(const Position &LHS, const Position &RHS) { Index: clangd/CodeComplete.cpp =================================================================== --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -19,6 +19,7 @@ #include "Compiler.h" #include "FuzzyMatch.h" #include "Logger.h" +#include "Quality.h" #include "SourceCode.h" #include "Trace.h" #include "index/Index.h" @@ -32,6 +33,9 @@ #include "llvm/Support/Format.h" #include <queue> +// We log detailed candidate here if you run with -debug-only=codecomplete. +#define DEBUG_TYPE "codecomplete" + namespace clang { namespace clangd { namespace { @@ -190,67 +194,14 @@ return Result; } -// Produces an integer that sorts in the same order as F. -// That is: a < b <==> encodeFloat(a) < encodeFloat(b). -uint32_t encodeFloat(float F) { - static_assert(std::numeric_limits<float>::is_iec559, ""); - static_assert(sizeof(float) == sizeof(uint32_t), ""); - constexpr uint32_t TopBit = ~(~uint32_t{0} >> 1); - - // Get the bits of the float. Endianness is the same as for integers. - uint32_t U; - memcpy(&U, &F, sizeof(float)); - // IEEE 754 floats compare like sign-magnitude integers. - if (U & TopBit) // Negative float. - return 0 - U; // Map onto the low half of integers, order reversed. - return U + TopBit; // Positive floats map onto the high half of integers. -} - -// Returns a string that sorts in the same order as (-Score, Name), for LSP. -std::string sortText(float Score, llvm::StringRef Name) { - // We convert -Score to an integer, and hex-encode for readability. - // Example: [0.5, "foo"] -> "41000000foo" - std::string S; - llvm::raw_string_ostream OS(S); - write_hex(OS, encodeFloat(-Score), llvm::HexPrintStyle::Lower, - /*Width=*/2 * sizeof(Score)); - OS << Name; - OS.flush(); - return S; -} - /// A code completion result, in clang-native form. /// It may be promoted to a CompletionItem if it's among the top-ranked results. struct CompletionCandidate { llvm::StringRef Name; // Used for filtering and sorting. // We may have a result from Sema, from the index, or both. const CodeCompletionResult *SemaResult = nullptr; const Symbol *IndexResult = nullptr; - // Computes the "symbol quality" score for this completion. Higher is better. - float score() const { - // For now we just use the Sema priority, mapping it onto a 0-1 interval. - if (!SemaResult) // FIXME(sammccall): better scoring for index results. - return 0.3f; // fixed mediocre score for index-only results. - - // Priority 80 is a really bad score. - float Score = 1 - std::min<float>(80, SemaResult->Priority) / 80; - - switch (static_cast<CXAvailabilityKind>(SemaResult->Availability)) { - case CXAvailability_Available: - // No penalty. - break; - case CXAvailability_Deprecated: - Score *= 0.1f; - break; - case CXAvailability_NotAccessible: - case CXAvailability_NotAvailable: - Score = 0; - break; - } - return Score; - } - // Builds an LSP completion item. CompletionItem build(llvm::StringRef FileName, const CompletionItemScores &Scores, @@ -316,6 +267,7 @@ return I; } }; +using ScoredCandidate = std::pair<CompletionCandidate, CompletionItemScores>; // Determine the symbol ID for a Sema code completion result, if possible. llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R) { @@ -522,50 +474,12 @@ UniqueFunction<void()> ResultsCallback; }; -// Tracks a bounded number of candidates with the best scores. -class TopN { -public: - using value_type = std::pair<CompletionCandidate, CompletionItemScores>; - static constexpr size_t Unbounded = std::numeric_limits<size_t>::max(); - - TopN(size_t N) : N(N) {} - - // Adds a candidate to the set. - // Returns true if a candidate was dropped to get back under N. - bool push(value_type &&V) { - bool Dropped = false; - if (Heap.size() >= N) { - Dropped = true; - if (N > 0 && greater(V, Heap.front())) { - std::pop_heap(Heap.begin(), Heap.end(), greater); - Heap.back() = std::move(V); - std::push_heap(Heap.begin(), Heap.end(), greater); - } - } else { - Heap.push_back(std::move(V)); - std::push_heap(Heap.begin(), Heap.end(), greater); - } - assert(Heap.size() <= N); - assert(std::is_heap(Heap.begin(), Heap.end(), greater)); - return Dropped; - } - - // Returns candidates from best to worst. - std::vector<value_type> items() && { - std::sort_heap(Heap.begin(), Heap.end(), greater); - assert(Heap.size() <= N); - return std::move(Heap); - } - -private: - static bool greater(const value_type &L, const value_type &R) { +struct ScoredCandidateGreater { + bool operator()(const ScoredCandidate &L, const ScoredCandidate &R) { if (L.second.finalScore != R.second.finalScore) return L.second.finalScore > R.second.finalScore; return L.first.Name < R.first.Name; // Earlier name is better. } - - const size_t N; - std::vector<value_type> Heap; // Min-heap, comparator is greater(). }; class SignatureHelpCollector final : public CodeCompleteConsumer { @@ -729,8 +643,15 @@ FrontendOpts.SkipFunctionBodies = true; FrontendOpts.CodeCompleteOpts = Options; FrontendOpts.CodeCompletionAt.FileName = Input.FileName; - FrontendOpts.CodeCompletionAt.Line = Input.Pos.line + 1; - FrontendOpts.CodeCompletionAt.Column = Input.Pos.character + 1; + auto Offset = positionToOffset(Input.Contents, Input.Pos); + if (!Offset) { + log("Code completion position was invalid " + + llvm::toString(Offset.takeError())); + return false; + } + std::tie(FrontendOpts.CodeCompletionAt.Line, + FrontendOpts.CodeCompletionAt.Column) = + offsetToClangLineColumn(Input.Contents, *Offset); Clang->setCodeCompletionConsumer(Consumer.release()); @@ -938,7 +859,8 @@ const SymbolSlab &IndexResults) { trace::Span Tracer("Merge and score results"); // We only keep the best N results at any time, in "native" format. - TopN Top(Opts.Limit == 0 ? TopN::Unbounded : Opts.Limit); + TopN<ScoredCandidate, ScoredCandidateGreater> Top( + Opts.Limit == 0 ? std::numeric_limits<size_t>::max() : Opts.Limit); llvm::DenseSet<const Symbol *> UsedIndexResults; auto CorrespondingIndexResult = [&](const CodeCompletionResult &SemaResult) -> const Symbol * { @@ -964,23 +886,42 @@ } // Scores a candidate and adds it to the TopN structure. - void addCandidate(TopN &Candidates, const CodeCompletionResult *SemaResult, + void addCandidate(TopN<ScoredCandidate, ScoredCandidateGreater> &Candidates, + const CodeCompletionResult *SemaResult, const Symbol *IndexResult) { CompletionCandidate C; C.SemaResult = SemaResult; C.IndexResult = IndexResult; C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult); - CompletionItemScores Scores; + SymbolQualitySignals Quality; + SymbolRelevanceSignals Relevance; if (auto FuzzyScore = Filter->match(C.Name)) - Scores.filterScore = *FuzzyScore; + Relevance.NameMatch = *FuzzyScore; else return; - Scores.symbolScore = C.score(); - // We score candidates by multiplying symbolScore ("quality" of the result) - // with filterScore (how well it matched the query). - // This is sensitive to the distribution of both component scores! - Scores.finalScore = Scores.filterScore * Scores.symbolScore; + if (IndexResult) + Quality.merge(*IndexResult); + if (SemaResult) { + Quality.merge(*SemaResult); + Relevance.merge(*SemaResult); + } + + float QualScore = Quality.evaluate(), RelScore = Relevance.evaluate(); + CompletionItemScores Scores; + Scores.finalScore = evaluateSymbolAndRelevance(QualScore, RelScore); + // The purpose of exporting component scores is to allow NameMatch to be + // replaced on the client-side. So we export (NameMatch, final/NameMatch) + // rather than (RelScore, QualScore). + Scores.filterScore = Relevance.NameMatch; + Scores.symbolScore = + Scores.filterScore ? Scores.finalScore / Scores.filterScore : QualScore; + + DEBUG(llvm::dbgs() << "CodeComplete: " << C.Name + << (IndexResult ? " (index)" : "") + << (SemaResult ? " (sema)" : "") << " = " + << Scores.finalScore << "\n" + << Quality << Relevance << "\n"); NSema += bool(SemaResult); NIndex += bool(IndexResult); Index: clangd/ClangdUnit.h =================================================================== --- clangd/ClangdUnit.h +++ clangd/ClangdUnit.h @@ -173,8 +173,9 @@ }; /// Get the beginning SourceLocation at a specified \p Pos. +/// May be invalid if Pos is, or if there's no identifier. SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos, - const FileEntry *FE); + const FileID FID); /// For testing/debugging purposes. Note that this method deserializes all /// unserialized Decls, so use with care. Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -215,19 +215,6 @@ std::move(IncLocations)); } -namespace { - -SourceLocation getMacroArgExpandedLocation(const SourceManager &Mgr, - const FileEntry *FE, Position Pos) { - // The language server protocol uses zero-based line and column numbers. - // Clang uses one-based numbers. - SourceLocation InputLoc = - Mgr.translateFileLineCol(FE, Pos.line + 1, Pos.character + 1); - return Mgr.getMacroArgExpandedLocation(InputLoc); -} - -} // namespace - void ParsedAST::ensurePreambleDeclsDeserialized() { if (PreambleDeclsDeserialized || !Preamble) return; @@ -470,40 +457,34 @@ SourceLocation clangd::getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos, - const FileEntry *FE) { + const FileID FID) { const ASTContext &AST = Unit.getASTContext(); const SourceManager &SourceMgr = AST.getSourceManager(); - - SourceLocation InputLocation = - getMacroArgExpandedLocation(SourceMgr, FE, Pos); - if (Pos.character == 0) { - return InputLocation; - } - - // This handle cases where the position is in the middle of a token or right - // after the end of a token. In theory we could just use GetBeginningOfToken - // to find the start of the token at the input position, but this doesn't - // work when right after the end, i.e. foo|. - // So try to go back by one and see if we're still inside an identifier - // token. If so, Take the beginning of this token. - // (It should be the same identifier because you can't have two adjacent - // identifiers without another token in between.) - Position PosCharBehind = Pos; - --PosCharBehind.character; - - SourceLocation PeekBeforeLocation = - getMacroArgExpandedLocation(SourceMgr, FE, PosCharBehind); - Token Result; - if (Lexer::getRawToken(PeekBeforeLocation, Result, SourceMgr, - AST.getLangOpts(), false)) { - // getRawToken failed, just use InputLocation. - return InputLocation; + auto Offset = positionToOffset(SourceMgr.getBufferData(FID), Pos); + if (!Offset) { + log("getBeginningOfIdentifier: " + toString(Offset.takeError())); + return SourceLocation(); } - - if (Result.is(tok::raw_identifier)) { - return Lexer::GetBeginningOfToken(PeekBeforeLocation, SourceMgr, - AST.getLangOpts()); - } - - return InputLocation; + SourceLocation InputLoc = SourceMgr.getComposedLoc(FID, *Offset); + + // GetBeginningOfToken(pos) is almost what we want, but does the wrong thing + // if the cursor is at the end of the identifier. + // Instead, we lex at GetBeginningOfToken(pos - 1). The cases are: + // 1) at the beginning of an identifier, we'll be looking at something + // that isn't an identifier. + // 2) at the middle or end of an identifier, we get the identifier. + // 3) anywhere outside an identifier, we'll get some non-identifier thing. + // We can't actually distinguish cases 1 and 3, but returning the original + // location is correct for both! + if (*Offset == 0) // Case 1 or 3. + return SourceMgr.getMacroArgExpandedLocation(InputLoc); + SourceLocation Before = + SourceMgr.getMacroArgExpandedLocation(InputLoc.getLocWithOffset(-1)); + Before = Lexer::GetBeginningOfToken(Before, SourceMgr, AST.getLangOpts()); + Token Tok; + if (Before.isValid() && + !Lexer::getRawToken(Before, Tok, SourceMgr, AST.getLangOpts(), false) && + Tok.is(tok::raw_identifier)) + return Before; // Case 2. + return SourceMgr.getMacroArgExpandedLocation(InputLoc); // Case 1 or 3. } Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -232,14 +232,8 @@ RefactoringResultCollector ResultCollector; const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); - const FileEntry *FE = - SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); - if (!FE) - return CB(llvm::make_error<llvm::StringError>( - "rename called for non-added document", - llvm::errc::invalid_argument)); SourceLocation SourceLocationBeg = - clangd::getBeginningOfIdentifier(AST, Pos, FE); + clangd::getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID()); tooling::RefactoringRuleContext Context( AST.getASTContext().getSourceManager()); Context.setASTContext(AST.getASTContext()); @@ -361,11 +355,11 @@ void ClangdServer::findDefinitions(PathRef File, Position Pos, Callback<std::vector<Location>> CB) { auto FS = FSProvider.getFileSystem(); - auto Action = [Pos, FS](Callback<std::vector<Location>> CB, - llvm::Expected<InputsAndAST> InpAST) { + auto Action = [Pos, FS, this](Callback<std::vector<Location>> CB, + llvm::Expected<InputsAndAST> InpAST) { if (!InpAST) return CB(InpAST.takeError()); - CB(clangd::findDefinitions(InpAST->AST, Pos)); + CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get())); }; WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB))); Index: clangd/CMakeLists.txt =================================================================== --- clangd/CMakeLists.txt +++ clangd/CMakeLists.txt @@ -28,6 +28,7 @@ Logger.cpp Protocol.cpp ProtocolHandlers.cpp + Quality.cpp SourceCode.cpp Threading.cpp Trace.cpp Index: clangd/AST.cpp =================================================================== --- clangd/AST.cpp +++ clangd/AST.cpp @@ -32,7 +32,7 @@ // be "<scratch space>" // * symbols controlled and defined by a compile command-line option // `-DName=foo`, the spelling location will be "<command line>". - SpellingLoc = SM.getExpansionRange(D->getLocation()).first; + SpellingLoc = SM.getExpansionRange(D->getLocation()).getBegin(); } } return SpellingLoc; Index: clang-tidy/tool/ClangTidyMain.cpp =================================================================== --- clang-tidy/tool/ClangTidyMain.cpp +++ clang-tidy/tool/ClangTidyMain.cpp @@ -41,7 +41,6 @@ Checks: '-*,some-check' WarningsAsErrors: '' HeaderFilterRegex: '' - AnalyzeTemporaryDtors: false FormatStyle: none User: user CheckOptions: @@ -182,16 +181,6 @@ cl::init(false), cl::cat(ClangTidyCategory)); -static cl::opt<bool> AnalyzeTemporaryDtors("analyze-temporary-dtors", - cl::desc(R"( -Enable temporary destructor-aware analysis in -clang-analyzer- checks. -This option overrides the value read from a -.clang-tidy file. -)"), - cl::init(false), - cl::cat(ClangTidyCategory)); - static cl::opt<std::string> ExportFixes("export-fixes", cl::desc(R"( YAML file to store suggested fixes in. The stored fixes can be applied to the input source @@ -300,7 +289,6 @@ DefaultOptions.WarningsAsErrors = ""; DefaultOptions.HeaderFilterRegex = HeaderFilter; DefaultOptions.SystemHeaders = SystemHeaders; - DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors; DefaultOptions.FormatStyle = FormatStyle; DefaultOptions.User = llvm::sys::Process::GetEnv("USER"); // USERNAME is used on Windows. @@ -316,8 +304,6 @@ OverrideOptions.HeaderFilterRegex = HeaderFilter; if (SystemHeaders.getNumOccurrences() > 0) OverrideOptions.SystemHeaders = SystemHeaders; - if (AnalyzeTemporaryDtors.getNumOccurrences() > 0) - OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors; if (FormatStyle.getNumOccurrences() > 0) OverrideOptions.FormatStyle = FormatStyle; Index: clang-tidy/performance/NoexceptMoveConstructorCheck.cpp =================================================================== --- clang-tidy/performance/NoexceptMoveConstructorCheck.cpp +++ clang-tidy/performance/NoexceptMoveConstructorCheck.cpp @@ -47,27 +47,22 @@ if (isUnresolvedExceptionSpec(ProtoType->getExceptionSpecType())) return; - switch (ProtoType->getNoexceptSpec(*Result.Context)) { - case FunctionProtoType::NR_NoNoexcept: + if (!isNoexceptExceptionSpec(ProtoType->getExceptionSpecType())) { diag(Decl->getLocation(), "move %0s should be marked noexcept") << MethodType; // FIXME: Add a fixit. - break; - case FunctionProtoType::NR_Throw: - // Don't complain about nothrow(false), but complain on nothrow(expr) - // where expr evaluates to false. - if (const Expr *E = ProtoType->getNoexceptExpr()) { - if (isa<CXXBoolLiteralExpr>(E)) - break; + return; + } + + // Don't complain about nothrow(false), but complain on nothrow(expr) + // where expr evaluates to false. + if (ProtoType->canThrow() == CT_Can) { + Expr *E = ProtoType->getNoexceptExpr(); + if (!isa<CXXBoolLiteralExpr>(ProtoType->getNoexceptExpr())) { diag(E->getExprLoc(), "noexcept specifier on the move %0 evaluates to 'false'") << MethodType; } - break; - case FunctionProtoType::NR_Nothrow: - case FunctionProtoType::NR_Dependent: - case FunctionProtoType::NR_BadNoexcept: - break; } } } Index: clang-tidy/objc/PropertyDeclarationCheck.cpp =================================================================== --- clang-tidy/objc/PropertyDeclarationCheck.cpp +++ clang-tidy/objc/PropertyDeclarationCheck.cpp @@ -45,11 +45,17 @@ "ARGB", "ASCII", "BGRA", + "CA", + "CF", + "CG", + "CI", + "CV", "CMYK", "DNS", "FPS", "FTP", "GIF", + "GL", "GPS", "GUID", "HD", @@ -65,6 +71,7 @@ "LZW", "MDNS", "MIDI", + "NS", "OS", "PDF", "PIN", @@ -81,6 +88,7 @@ "RPC", "RTF", "RTL", + "SC", "SDK", "SSO", "TCP", Index: clang-tidy/modernize/UseNullptrCheck.cpp =================================================================== --- clang-tidy/modernize/UseNullptrCheck.cpp +++ clang-tidy/modernize/UseNullptrCheck.cpp @@ -332,7 +332,7 @@ NullMacros.end(); } - MacroLoc = SM.getExpansionRange(ArgLoc).first; + MacroLoc = SM.getExpansionRange(ArgLoc).getBegin(); ArgLoc = Expansion.getSpellingLoc().getLocWithOffset(LocInfo.second); if (ArgLoc.isFileID()) @@ -387,7 +387,7 @@ continue; } - MacroLoc = SM.getImmediateExpansionRange(Loc).first; + MacroLoc = SM.getImmediateExpansionRange(Loc).getBegin(); if (MacroLoc.isFileID() && MacroLoc == TestMacroLoc) { // Match made. return true; Index: clang-tidy/modernize/UseNoexceptCheck.cpp =================================================================== --- clang-tidy/modernize/UseNoexceptCheck.cpp +++ clang-tidy/modernize/UseNoexceptCheck.cpp @@ -89,7 +89,7 @@ Result.Context->getLangOpts()); assert(FnTy && "FunctionProtoType is null."); - bool IsNoThrow = FnTy->isNothrow(*Result.Context); + bool IsNoThrow = FnTy->isNothrow(); StringRef ReplacementStr = IsNoThrow ? NoexceptMacro.empty() ? "noexcept" : NoexceptMacro.c_str() Index: clang-tidy/modernize/RawStringLiteralCheck.h =================================================================== --- clang-tidy/modernize/RawStringLiteralCheck.h +++ clang-tidy/modernize/RawStringLiteralCheck.h @@ -11,11 +11,14 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_RAW_STRING_LITERAL_H #include "../ClangTidy.h" +#include <bitset> namespace clang { namespace tidy { namespace modernize { +using CharsBitSet = std::bitset<1 << CHAR_BIT>; + /// This check replaces string literals with escaped characters to /// raw string literals. /// @@ -35,6 +38,7 @@ const StringLiteral *Literal, StringRef Replacement); std::string DelimiterStem; + CharsBitSet DisallowedChars; const bool ReplaceShorterLiterals; }; Index: clang-tidy/modernize/RawStringLiteralCheck.cpp =================================================================== --- clang-tidy/modernize/RawStringLiteralCheck.cpp +++ clang-tidy/modernize/RawStringLiteralCheck.cpp @@ -42,28 +42,15 @@ } bool containsEscapedCharacters(const MatchFinder::MatchResult &Result, - const StringLiteral *Literal) { + const StringLiteral *Literal, + const CharsBitSet &DisallowedChars) { // FIXME: Handle L"", u8"", u"" and U"" literals. if (!Literal->isAscii()) return false; - StringRef Bytes = Literal->getBytes(); - // Non-printing characters disqualify this literal: - // \007 = \a bell - // \010 = \b backspace - // \011 = \t horizontal tab - // \012 = \n new line - // \013 = \v vertical tab - // \014 = \f form feed - // \015 = \r carriage return - // \177 = delete - if (Bytes.find_first_of(StringRef("\000\001\002\003\004\005\006\a" - "\b\t\n\v\f\r\016\017" - "\020\021\022\023\024\025\026\027" - "\030\031\032\033\034\035\036\037" - "\177", - 33)) != StringRef::npos) - return false; + for (const unsigned char C : Literal->getBytes()) + if (DisallowedChars.test(C)) + return false; CharSourceRange CharRange = Lexer::makeFileCharRange( CharSourceRange::getTokenRange(Literal->getSourceRange()), @@ -102,7 +89,28 @@ ClangTidyContext *Context) : ClangTidyCheck(Name, Context), DelimiterStem(Options.get("DelimiterStem", "lit")), - ReplaceShorterLiterals(Options.get("ReplaceShorterLiterals", false)) {} + ReplaceShorterLiterals(Options.get("ReplaceShorterLiterals", false)) { + // Non-printing characters are disallowed: + // \007 = \a bell + // \010 = \b backspace + // \011 = \t horizontal tab + // \012 = \n new line + // \013 = \v vertical tab + // \014 = \f form feed + // \015 = \r carriage return + // \177 = delete + for (const unsigned char C : StringRef("\000\001\002\003\004\005\006\a" + "\b\t\n\v\f\r\016\017" + "\020\021\022\023\024\025\026\027" + "\030\031\032\033\034\035\036\037" + "\177", + 33)) + DisallowedChars.set(C); + + // Non-ASCII are disallowed too. + for (unsigned int C = 0x80u; C <= 0xFFu; ++C) + DisallowedChars.set(static_cast<unsigned char>(C)); +} void RawStringLiteralCheck::storeOptions(ClangTidyOptions::OptionMap &Options) { ClangTidyCheck::storeOptions(Options); @@ -124,7 +132,7 @@ if (Literal->getLocStart().isMacroID()) return; - if (containsEscapedCharacters(Result, Literal)) { + if (containsEscapedCharacters(Result, Literal, DisallowedChars)) { std::string Replacement = asRawStringLiteral(Literal, DelimiterStem); if (ReplaceShorterLiterals || Replacement.length() <= Index: clang-tidy/google/IntegerTypesCheck.cpp =================================================================== --- clang-tidy/google/IntegerTypesCheck.cpp +++ clang-tidy/google/IntegerTypesCheck.cpp @@ -11,6 +11,7 @@ #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/AttrKinds.h" #include "clang/Basic/CharInfo.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/TargetInfo.h" @@ -56,7 +57,16 @@ // Find all TypeLocs. The relevant Style Guide rule only applies to C++. if (!getLangOpts().CPlusPlus) return; - Finder->addMatcher(typeLoc(loc(isInteger())).bind("tl"), this); + // Match any integer types, unless they are passed to a printf-based API: + // + // http://google.github.io/styleguide/cppguide.html#64-bit_Portability + // "Where possible, avoid passing arguments of types specified by + // bitwidth typedefs to printf-based APIs." + Finder->addMatcher(typeLoc(loc(isInteger()), + unless(hasAncestor(callExpr( + callee(functionDecl(hasAttr(attr::Format))))))) + .bind("tl"), + this); IdentTable = llvm::make_unique<IdentifierTable>(getLangOpts()); } Index: clang-tidy/bugprone/MultipleStatementMacroCheck.cpp =================================================================== --- clang-tidy/bugprone/MultipleStatementMacroCheck.cpp +++ clang-tidy/bugprone/MultipleStatementMacroCheck.cpp @@ -38,17 +38,18 @@ return nextStmt(Result, Parent); } -using ExpansionRanges = std::vector<std::pair<SourceLocation, SourceLocation>>; +using ExpansionRanges = std::vector<SourceRange>; /// \bried Get all the macro expansion ranges related to `Loc`. /// /// The result is ordered from most inner to most outer. ExpansionRanges getExpansionRanges(SourceLocation Loc, const MatchFinder::MatchResult &Result) { ExpansionRanges Locs; while (Loc.isMacroID()) { - Locs.push_back(Result.SourceManager->getImmediateExpansionRange(Loc)); - Loc = Locs.back().first; + Locs.push_back( + Result.SourceManager->getImmediateExpansionRange(Loc).getAsRange()); + Loc = Locs.back().getBegin(); } return Locs; } @@ -96,9 +97,9 @@ InnerRanges.back() != NextRanges.back()) return; - diag(InnerRanges.back().first, "multiple statement macro used without " - "braces; some statements will be " - "unconditionally executed"); + diag(InnerRanges.back().getBegin(), "multiple statement macro used without " + "braces; some statements will be " + "unconditionally executed"); } } // namespace bugprone Index: clang-tidy/bugprone/LambdaFunctionNameCheck.cpp =================================================================== --- clang-tidy/bugprone/LambdaFunctionNameCheck.cpp +++ clang-tidy/bugprone/LambdaFunctionNameCheck.cpp @@ -81,7 +81,7 @@ if (E->getLocation().isMacroID()) { auto ER = Result.SourceManager->getImmediateExpansionRange(E->getLocation()); - if (SuppressMacroExpansions.find(SourceRange(ER.first, ER.second)) != + if (SuppressMacroExpansions.find(ER.getAsRange()) != SuppressMacroExpansions.end()) { // This is a macro expansion for which we should not warn. return; Index: clang-tidy/ClangTidyOptions.h =================================================================== --- clang-tidy/ClangTidyOptions.h +++ clang-tidy/ClangTidyOptions.h @@ -74,9 +74,6 @@ /// \brief Output warnings from system headers matching \c HeaderFilterRegex. llvm::Optional<bool> SystemHeaders; - /// \brief Turns on temporary destructor-based analysis. - llvm::Optional<bool> AnalyzeTemporaryDtors; - /// \brief Format code around applied fixes with clang-format using this /// style. /// Index: clang-tidy/ClangTidyOptions.cpp =================================================================== --- clang-tidy/ClangTidyOptions.cpp +++ clang-tidy/ClangTidyOptions.cpp @@ -86,7 +86,6 @@ IO.mapOptional("Checks", Options.Checks); IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors); IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex); - IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors); IO.mapOptional("FormatStyle", Options.FormatStyle); IO.mapOptional("User", Options.User); IO.mapOptional("CheckOptions", NOpts->Options); @@ -107,7 +106,6 @@ Options.WarningsAsErrors = ""; Options.HeaderFilterRegex = ""; Options.SystemHeaders = false; - Options.AnalyzeTemporaryDtors = false; Options.FormatStyle = "none"; Options.User = llvm::None; for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(), @@ -147,7 +145,6 @@ mergeCommaSeparatedLists(Result.WarningsAsErrors, Other.WarningsAsErrors); overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex); overrideValue(Result.SystemHeaders, Other.SystemHeaders); - overrideValue(Result.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors); overrideValue(Result.FormatStyle, Other.FormatStyle); overrideValue(Result.User, Other.User); mergeVectors(Result.ExtraArgs, Other.ExtraArgs); Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp =================================================================== --- clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -374,7 +374,7 @@ return true; if (!Loc.isMacroID()) return false; - Loc = SM.getImmediateExpansionRange(Loc).first; + Loc = SM.getImmediateExpansionRange(Loc).getBegin(); } return false; } Index: clang-tidy/ClangTidy.cpp =================================================================== --- clang-tidy/ClangTidy.cpp +++ clang-tidy/ClangTidy.cpp @@ -369,11 +369,6 @@ Consumers.push_back(Finder->newASTConsumer()); AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts(); - // FIXME: Remove this option once clang's cfg-temporary-dtors option defaults - // to true. - AnalyzerOptions->Config["cfg-temporary-dtors"] = - Context.getOptions().AnalyzeTemporaryDtors ? "true" : "false"; - AnalyzerOptions->CheckersControlList = getCheckersControlList(Context); if (!AnalyzerOptions->CheckersControlList.empty()) { setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions); Index: clang-move/ClangMove.cpp =================================================================== --- clang-move/ClangMove.cpp +++ clang-move/ClangMove.cpp @@ -280,7 +280,10 @@ getLocForEndOfDecl(const clang::Decl *D, const LangOptions &LangOpts = clang::LangOptions()) { const auto &SM = D->getASTContext().getSourceManager(); - auto EndExpansionLoc = SM.getExpansionRange(D->getLocEnd()).second; + // If the expansion range is a character range, this is the location of + // the first character past the end. Otherwise it's the location of the + // first character in the final token in the range. + auto EndExpansionLoc = SM.getExpansionRange(D->getLocEnd()).getEnd(); std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(EndExpansionLoc); // Try to load the file buffer. bool InvalidTemp = false;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits