[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec
elsteveogrande created this revision.
elsteveogrande added reviewers: rsmith, dblaikie.
Herald added a subscriber: cfe-commits.
(PR38627)
After deserializing, the `PendingExceptionSpecUpdates` table is iterated over
to apply exception specs to functions. There may be `EST_None`-type in this
table. However if there is a method in the redecl chain already having some
other non-`EST_None` type, this should not be clobbered with none.
If we see `EST_None` avoid setting the exception spec. (This is the default
anyway, so it's safe to skip in the legit case.)
There may be *better* ways to actually fix this, rather than a simple 1-line
defensive check like I did here. It would be more complicated and quite
outside my area of experience though.
Test Plan: Passes `ninja check-clang`, including a new test case.
Andrew Gallagher provided the test case (distilling it from a much more complex
instance in our code base).
Repository:
rC Clang
https://reviews.llvm.org/D51608
Files:
lib/Serialization/ASTReader.cpp
test/Modules/Inputs/PR38627/a.h
test/Modules/Inputs/PR38627/module.modulemap
test/Modules/pr38627.cpp
Index: test/Modules/pr38627.cpp
===
--- /dev/null
+++ test/Modules/pr38627.cpp
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: %clang -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/PR38627 -c %s
+// expected-no-diagnostics
+
+// PR38627: Exception specifications sometimes go missing / are incorrect
+// after deserializing from a module.
+// https://bugs.llvm.org/show_bug.cgi?id=38627
+
+namespace test_with_unserialized_decls {
+
+// X/Y/Z are the same as A/B/C from: Inputs/PR38627/a.h
+class X {
+ public:
+ virtual ~X() = default;
+};
+class Y {
+ public:
+ virtual ~Y() {
+z.func();
+ }
+ struct Z {
+void func() {}
+friend Y::~Y();
+ };
+ Z z;
+};
+
+// ~Y's exception spec should be calculated to be noexcept.
+static_assert(noexcept(Y().~Y()));
+
+} // end test_with_unserialized_decls
+
+// Same definitions of A and B (but without the namespace) in this module
+#include "a.h"
+
+namespace test_with_deserialized_decls {
+
+static_assert(noexcept(B().~B()), "PR38627");
+
+class D : public A, public B {
+ public:
+ // Error should be gone with bug fix:
+ // "exception specification of overriding function is more lax than base version"
+ virtual ~D() override = default;
+};
+
+} // end test_with_deserialized_decls
Index: test/Modules/Inputs/PR38627/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/PR38627/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/a.h
@@ -0,0 +1,17 @@
+#pragma once
+
+class A {
+ public:
+ virtual ~A() = default;
+};
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+ struct C {
+void func() {}
+friend B::~B();
+ };
+ C c;
+};
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -11551,10 +11551,17 @@
ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
auto *FPT = Update.second->getType()->castAs();
auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
-if (auto *Listener = getContext().getASTMutationListener())
- Listener->ResolvedExceptionSpec(cast(Update.second));
-for (auto *Redecl : Update.second->redecls())
- getContext().adjustExceptionSpec(cast(Redecl), ESI);
+// Ignore updates with EST_None sorts of exception specifications.
+// That is the default anyway; also, in the event a FunctionDecl does
+// have a non-None type, we'll avoid clobbering it.
+if (ESI.Type != ExceptionSpecificationType::EST_None) {
+ if (auto *Listener = getContext().getASTMutationListener())
+Listener->ResolvedExceptionSpec(cast(Update.second));
+ for (auto *Redecl : Update.second->redecls()) {
+// FIXME: If the exception spec is already set, ensure it matches.
+getContext().adjustExceptionSpec(cast(Redecl), ESI);
+ }
+}
}
auto DTUpdates = std::move(PendingDeducedTypeUpdates);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup
sammccall updated this revision to Diff 163751.
sammccall added a comment.
Remove RefsSlab::find()
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51605
Files:
clangd/ClangdServer.cpp
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/dex/DexIndex.cpp
clangd/index/dex/DexIndex.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -49,8 +49,8 @@
}
std::unique_ptr TestTU::index() const {
- // FIXME: we should generate proper occurrences for TestTU.
- return MemIndex::build(headerSymbols(), SymbolOccurrenceSlab::createEmpty());
+ // FIXME: we should generate proper refs for TestTU.
+ return MemIndex::build(headerSymbols(), RefSlab());
}
const Symbol (const SymbolSlab , llvm::StringRef QName) {
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -33,11 +33,14 @@
namespace {
+using testing::_;
using testing::AllOf;
+using testing::Contains;
using testing::Eq;
using testing::Field;
using testing::IsEmpty;
using testing::Not;
+using testing::Pair;
using testing::UnorderedElementsAre;
using testing::UnorderedElementsAreArray;
@@ -76,21 +79,21 @@
std::tie(Pos.start.line, Pos.start.character, Pos.end.line,
Pos.end.character);
}
-MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
}
-MATCHER(OccurrenceRange, "") {
- const SymbolOccurrence = testing::get<0>(arg);
+MATCHER(RefRange, "") {
+ const Ref = testing::get<0>(arg);
const Range = testing::get<1>(arg);
return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
Pos.Location.End.Line, Pos.Location.End.Column) ==
std::tie(Range.start.line, Range.start.character, Range.end.line,
Range.end.character);
}
-testing::Matcher &>
+testing::Matcher &>
HaveRanges(const std::vector Ranges) {
- return testing::UnorderedPointwise(OccurrenceRange(), Ranges);
+ return testing::UnorderedPointwise(RefRange(), Ranges);
}
class ShouldCollectSymbolTest : public ::testing::Test {
@@ -250,7 +253,7 @@
llvm::MemoryBuffer::getMemBuffer(MainCode));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
-SymbolOccurrences = Factory->Collector->takeOccurrences();
+Refs = Factory->Collector->takeRefs();
return true;
}
@@ -261,7 +264,7 @@
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
- SymbolOccurrenceSlab SymbolOccurrences;
+ RefSlab Refs;
SymbolCollector::Options CollectorOpts;
std::unique_ptr PragmaHandler;
};
@@ -428,7 +431,7 @@
));
}
-TEST_F(SymbolCollectorTest, Occurrences) {
+TEST_F(SymbolCollectorTest, Refs) {
Annotations Header(R"(
class $foo[[Foo]] {
public:
@@ -457,28 +460,23 @@
static const int c = 0;
class d {};
)");
- CollectorOpts.OccurrenceFilter = AllOccurrenceKinds;
+ CollectorOpts.RefFilter = RefKind::All;
runSymbolCollector(Header.code(),
(Main.code() + SymbolsOnlyInMainCode.code()).str());
auto HeaderSymbols = TestTU::withHeaderCode(Header.code()).headerSymbols();
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
- HaveRanges(Main.ranges("foo")));
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "Bar").ID),
- HaveRanges(Main.ranges("bar")));
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "func").ID),
- HaveRanges(Main.ranges("func")));
-
- // Retrieve IDs for symbols *only* in the main file, and verify these symbols
- // are not collected.
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+ HaveRanges(Main.ranges("foo");
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Bar").ID,
+ HaveRanges(Main.ranges("bar");
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "func").ID,
+ HaveRanges(Main.ranges("func");
+ // Symbols *only* in the main file (a, b, c) had no refs collected.
auto MainSymbols =
[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous,
MaskRay, ilya-biryukov.
A few things that I noticed while merging the SwapIndex patch:
- SymbolOccurrences and particularly SymbolOccurrenceSlab are unwieldy names,
and these names appear *a lot*. Ref, RefSlab, etc seem clear enough and
read/format much better.
- The asymmetry between SymbolSlab and RefSlab (build() vs freeze()) is
confusing and irritating, and doesn't even save much code. Avoiding
RefSlab::Builder was my idea, but it was a bad one; add it.
- DenseMap> seems like a reasonable compromise for
constructing MemIndex - and means many less wasted allocations than the current
DenseMap> for FileIndex, and none for slabs.
- A few naming/consistency fixes: e.g. Slabs,Refs -> Symbols,Refs.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51605
Files:
clangd/ClangdServer.cpp
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/dex/DexIndex.cpp
clangd/index/dex/DexIndex.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -49,8 +49,8 @@
}
std::unique_ptr TestTU::index() const {
- // FIXME: we should generate proper occurrences for TestTU.
- return MemIndex::build(headerSymbols(), SymbolOccurrenceSlab::createEmpty());
+ // FIXME: we should generate proper refs for TestTU.
+ return MemIndex::build(headerSymbols(), RefSlab());
}
const Symbol (const SymbolSlab , llvm::StringRef QName) {
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -76,21 +76,21 @@
std::tie(Pos.start.line, Pos.start.character, Pos.end.line,
Pos.end.character);
}
-MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
}
-MATCHER(OccurrenceRange, "") {
- const SymbolOccurrence = testing::get<0>(arg);
+MATCHER(RefRange, "") {
+ const Ref = testing::get<0>(arg);
const Range = testing::get<1>(arg);
return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
Pos.Location.End.Line, Pos.Location.End.Column) ==
std::tie(Range.start.line, Range.start.character, Range.end.line,
Range.end.character);
}
-testing::Matcher &>
+testing::Matcher &>
HaveRanges(const std::vector Ranges) {
- return testing::UnorderedPointwise(OccurrenceRange(), Ranges);
+ return testing::UnorderedPointwise(RefRange(), Ranges);
}
class ShouldCollectSymbolTest : public ::testing::Test {
@@ -250,7 +250,7 @@
llvm::MemoryBuffer::getMemBuffer(MainCode));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
-SymbolOccurrences = Factory->Collector->takeOccurrences();
+Refs = Factory->Collector->takeRefs();
return true;
}
@@ -261,7 +261,7 @@
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
- SymbolOccurrenceSlab SymbolOccurrences;
+ RefSlab Refs;
SymbolCollector::Options CollectorOpts;
std::unique_ptr PragmaHandler;
};
@@ -428,7 +428,7 @@
));
}
-TEST_F(SymbolCollectorTest, Occurrences) {
+TEST_F(SymbolCollectorTest, Refs) {
Annotations Header(R"(
class $foo[[Foo]] {
public:
@@ -457,28 +457,25 @@
static const int c = 0;
class d {};
)");
- CollectorOpts.OccurrenceFilter = AllOccurrenceKinds;
+ CollectorOpts.RefFilter = RefKind::All;
runSymbolCollector(Header.code(),
(Main.code() + SymbolsOnlyInMainCode.code()).str());
auto HeaderSymbols = TestTU::withHeaderCode(Header.code()).headerSymbols();
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+ EXPECT_THAT(Refs.find(findSymbol(Symbols, "Foo").ID),
HaveRanges(Main.ranges("foo")));
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "Bar").ID),
+ EXPECT_THAT(Refs.find(findSymbol(Symbols, "Bar").ID),
HaveRanges(Main.ranges("bar")));
- EXPECT_THAT(SymbolOccurrences.find(findSymbol(Symbols, "func").ID),
+ EXPECT_THAT(Refs.find(findSymbol(Symbols, "func").ID),
[PATCH] D51573: [Driver] Search LibraryPaths when handling -print-file-name
phosek added inline comments.
Comment at: clang/lib/Driver/Driver.cpp:4187
SmallString<128> R(ResourceDir);
llvm::sys::path::append(R, Name);
mcgrathr wrote:
> You could fold these cases into the lambda too by having it take a bool
> UseSysRoot and passing `{ResourceDir}, false`.
> Then the main body is a uniform and concise list of paths to check.
I considered that but `llvm::SmallVector` doesn't support initializer list so
this ends up looking not so great.
Repository:
rC Clang
https://reviews.llvm.org/D51573
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r341337 - [clangd] Fix index-twice regression from r341242
Author: sammccall Date: Mon Sep 3 13:26:26 2018 New Revision: 341337 URL: http://llvm.org/viewvc/llvm-project?rev=341337=rev Log: [clangd] Fix index-twice regression from r341242 Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=341337=341336=341337=diff == --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original) +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Mon Sep 3 13:26:26 2018 @@ -125,7 +125,6 @@ void FileIndex::update(PathRef Path, AST assert(PP); auto Slab = llvm::make_unique(); auto OccurrenceSlab = llvm::make_unique(); -auto IndexResults = indexAST(*AST, PP, TopLevelDecls, URISchemes); std::tie(*Slab, *OccurrenceSlab) = indexAST(*AST, PP, TopLevelDecls, URISchemes); FSymbols.update(Path, std::move(Slab), std::move(OccurrenceSlab)); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall updated this revision to Diff 163744.
sammccall added a comment.
Fix comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51585
Files:
clangd/CMakeLists.txt
clangd/RIFF.cpp
clangd/RIFF.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/RIFFTests.cpp
unittests/clangd/SerializationTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -45,7 +45,6 @@
MATCHER_P(Labeled, Label, "") {
return (arg.Name + arg.Signature).str() == Label;
}
-MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
MATCHER_P(Snippet, S, "") {
@@ -746,84 +745,6 @@
Snippet("ff(${1:int x}, ${2:double y})";
}
-TEST_F(SymbolCollectorTest, YAMLConversions) {
- const std::string YAML1 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
-Name: 'Foo1'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/foo.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:true
-Documentation:'Foo doc'
-ReturnType:'int'
-IncludeHeaders:
- - Header:'include1'
-References:7
- - Header:'include2'
-References:3
-...
-)";
- const std::string YAML2 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
-Name: 'Foo2'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/bar.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:false
-Signature:'-sig'
-CompletionSnippetSuffix:'-snippet'
-...
-)";
-
- auto Symbols1 = symbolsFromYAML(YAML1);
-
- EXPECT_THAT(Symbols1,
- UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
- Doc("Foo doc"), ReturnType("int"),
- DeclURI("file:///path/foo.h"),
- ForCodeCompletion(true;
- auto = *Symbols1.begin();
- EXPECT_THAT(Sym1.IncludeHeaders,
- UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
- IncludeHeaderWithRef("include2", 3u)));
- auto Symbols2 = symbolsFromYAML(YAML2);
- EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
-QName("clang::Foo2"), Labeled("Foo2-sig"),
-Not(HasReturnType()), DeclURI("file:///path/bar.h"),
-ForCodeCompletion(false;
-
- std::string ConcatenatedYAML;
- {
-llvm::raw_string_ostream OS(ConcatenatedYAML);
-SymbolsToYAML(Symbols1, OS);
-SymbolsToYAML(Symbols2, OS);
- }
- auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
- EXPECT_THAT(ConcatenatedSymbols,
- UnorderedElementsAre(QName("clang::Foo1"),
- QName("clang::Foo2")));
-}
-
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
Index: unittests/clangd/SerializationTests.cpp
===
--- /dev/null
+++ unittests/clangd/SerializationTests.cpp
@@ -0,0 +1,138 @@
+//===-- SerializationTests.cpp - Binary and YAML serialization unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/Serialization.h"
+#include "index/SymbolYAML.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
+namespace clang {
+namespace clangd {
+namespace {
+
+const char *YAML1 = R"(
+---
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
+Name: 'Foo1'
+Scope: 'clang::'
+SymInfo:
+ Kind:Function
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI:file:///path/foo.h
+ Start:
+Line: 1
+Column: 0
+ End:
+Line: 1
+Column: 1
+IsIndexedForCodeCompletion:true
+Documentation:'Foo doc'
+ReturnType:'int'
+IncludeHeaders:
+ - Header:'include1'
+References:7
+ - Header:'include2'
+References:
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall updated this revision to Diff 163743.
sammccall added a comment.
Fix subtle macro expansion bug!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51585
Files:
clangd/CMakeLists.txt
clangd/RIFF.cpp
clangd/RIFF.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/RIFFTests.cpp
unittests/clangd/SerializationTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -45,7 +45,6 @@
MATCHER_P(Labeled, Label, "") {
return (arg.Name + arg.Signature).str() == Label;
}
-MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
MATCHER_P(Snippet, S, "") {
@@ -746,84 +745,6 @@
Snippet("ff(${1:int x}, ${2:double y})";
}
-TEST_F(SymbolCollectorTest, YAMLConversions) {
- const std::string YAML1 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
-Name: 'Foo1'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/foo.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:true
-Documentation:'Foo doc'
-ReturnType:'int'
-IncludeHeaders:
- - Header:'include1'
-References:7
- - Header:'include2'
-References:3
-...
-)";
- const std::string YAML2 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
-Name: 'Foo2'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/bar.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:false
-Signature:'-sig'
-CompletionSnippetSuffix:'-snippet'
-...
-)";
-
- auto Symbols1 = symbolsFromYAML(YAML1);
-
- EXPECT_THAT(Symbols1,
- UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
- Doc("Foo doc"), ReturnType("int"),
- DeclURI("file:///path/foo.h"),
- ForCodeCompletion(true;
- auto = *Symbols1.begin();
- EXPECT_THAT(Sym1.IncludeHeaders,
- UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
- IncludeHeaderWithRef("include2", 3u)));
- auto Symbols2 = symbolsFromYAML(YAML2);
- EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
-QName("clang::Foo2"), Labeled("Foo2-sig"),
-Not(HasReturnType()), DeclURI("file:///path/bar.h"),
-ForCodeCompletion(false;
-
- std::string ConcatenatedYAML;
- {
-llvm::raw_string_ostream OS(ConcatenatedYAML);
-SymbolsToYAML(Symbols1, OS);
-SymbolsToYAML(Symbols2, OS);
- }
- auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
- EXPECT_THAT(ConcatenatedSymbols,
- UnorderedElementsAre(QName("clang::Foo1"),
- QName("clang::Foo2")));
-}
-
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
Index: unittests/clangd/SerializationTests.cpp
===
--- /dev/null
+++ unittests/clangd/SerializationTests.cpp
@@ -0,0 +1,138 @@
+//===-- SerializationTests.cpp - Binary and YAML serialization unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/Serialization.h"
+#include "index/SymbolYAML.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
+namespace clang {
+namespace clangd {
+namespace {
+
+const char *YAML1 = R"(
+---
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
+Name: 'Foo1'
+Scope: 'clang::'
+SymInfo:
+ Kind:Function
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI:file:///path/foo.h
+ Start:
+Line: 1
+Column: 0
+ End:
+Line: 1
+Column: 1
+IsIndexedForCodeCompletion:true
+Documentation:'Foo doc'
+ReturnType:'int'
+IncludeHeaders:
+ - Header:'include1'
+References:7
+ - Header:
[PATCH] D50515: Re-push "[Option] Fix PR37006 prefix choice in findNearest"
modocache added a comment. Excellent, I think pushing this along with https://reviews.llvm.org/D50410 revealed the true error, as an MSAN buildbot tells use there's a use of an uninitialized value: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/23176/steps/check-clang%20msan/logs/stdio I haven't looked into it a ton yet but it seems promising. However, for the time being, I reverted this change in https://reviews.llvm.org/rL341333. Repository: rL LLVM https://reviews.llvm.org/D50515 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
usaxena95 added a comment.
Applied the changes suggested.
Comment at: lib/Basic/VirtualFileSystem.cpp:677
+if (HardLinkTarget)
+ Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget));
+else {
ilya-biryukov wrote:
> NIT: `.str()` seems redundant
Here P is a Twine here which cannot be converted to StringRef
Repository:
rC Clang
https://reviews.llvm.org/D51359
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
usaxena95 updated this revision to Diff 163739.
usaxena95 marked 14 inline comments as done.
usaxena95 added a comment.
- applied changes
Repository:
rC Clang
https://reviews.llvm.org/D51359
Files:
include/clang/Basic/VirtualFileSystem.h
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -16,7 +16,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/SourceMgr.h"
#include "gtest/gtest.h"
+#include "gmock/gmock.h"
#include
+#include
using namespace clang;
using namespace llvm;
@@ -697,6 +699,16 @@
NormalizedFS(/*UseNormalizedPaths=*/true) {}
};
+MATCHER_P2(IsHardLinkTo, FS, Target, "") {
+ StringRef From = arg;
+ StringRef To = Target;
+ auto OpenedFrom = FS->openFileForRead(From);
+ auto OpenedTo = FS->openFileForRead(To);
+ return !(OpenedFrom.getError() || OpenedTo.getError() ||
+ (*OpenedFrom)->status()->getUniqueID() !=
+ (*OpenedTo)->status()->getUniqueID());
+}
+
TEST_F(InMemoryFileSystemTest, IsEmpty) {
auto Stat = FS.status("/a");
ASSERT_EQ(Stat.getError(),errc::no_such_file_or_directory) << FS.toString();
@@ -958,6 +970,108 @@
ASSERT_EQ("../b/c", getPosixPath(It->getName()));
}
+TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) {
+ StringRef FromLink = "/path/to/FROM/link";
+ StringRef Target = "/path/to/TO/file";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer("content of target"));
+ EXPECT_TRUE(FS.addHardLink(FromLink, Target));
+ EXPECT_THAT(FromLink, IsHardLinkTo(, Target));
+ EXPECT_TRUE(FS.status(FromLink)->getSize() == FS.status(Target)->getSize());
+ EXPECT_TRUE(FS.getBufferForFile(FromLink)->get()->getBuffer() ==
+ FS.getBufferForFile(Target)->get()->getBuffer());
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkInChainPattern) {
+ StringRef Link0 = "/path/to/0/link";
+ StringRef Link1 = "/path/to/1/link";
+ StringRef Link2 = "/path/to/2/link";
+ StringRef Target = "/path/to/target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer("content of target file"));
+ EXPECT_TRUE(FS.addHardLink(Link2, Target));
+ EXPECT_TRUE(FS.addHardLink(Link1, Link2));
+ EXPECT_TRUE(FS.addHardLink(Link0, Link1));
+ EXPECT_THAT(Link0, IsHardLinkTo(, Target));
+ EXPECT_THAT(Link1, IsHardLinkTo(, Target));
+ EXPECT_THAT(Link2, IsHardLinkTo(, Target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToAFileThatWasNotAddedBefore) {
+ EXPECT_FALSE(FS.addHardLink("/path/to/link", "/path/to/target"));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromAFileThatWasAddedBefore) {
+ StringRef Link = "/path/to/link";
+ StringRef Target = "/path/to/target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer("content of target"));
+ FS.addFile(Link, 0, MemoryBuffer::getMemBuffer("content of link"));
+ EXPECT_FALSE(FS.addHardLink(Link, Target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddSameHardLinkMoreThanOnce) {
+ StringRef Link = "/path/to/link";
+ StringRef Target = "/path/to/target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer("content of target"));
+ EXPECT_TRUE(FS.addHardLink(Link, Target));
+ EXPECT_FALSE(FS.addHardLink(Link, Target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithSameContent) {
+ StringRef Link = "/path/to/link";
+ StringRef Target = "/path/to/target";
+ StringRef Content = "content of target";
+ EXPECT_TRUE(FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(Content)));
+ EXPECT_TRUE(FS.addHardLink(Link, Target));
+ EXPECT_TRUE(FS.addFile(Link, 0, MemoryBuffer::getMemBuffer(Content)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithDifferentContent) {
+ StringRef Link = "/path/to/link";
+ StringRef Target = "/path/to/target";
+ StringRef Content = "content of target";
+ StringRef LinkContent = "different content of link";
+ EXPECT_TRUE(FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(Content)));
+ EXPECT_TRUE(FS.addHardLink(Link, Target));
+ EXPECT_FALSE(FS.addFile(Link, 0, MemoryBuffer::getMemBuffer(LinkContent)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToADirectory) {
+ StringRef Dir = "path/to/dummy/dir";
+ StringRef Link = "/path/to/link";
+ StringRef File = "path/to/dummy/dir/target";
+ StringRef Content = "content of target";
+ EXPECT_TRUE(FS.addFile(File, 0, MemoryBuffer::getMemBuffer(Content)));
+ EXPECT_FALSE(FS.addHardLink(Link, Dir));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromADirectory) {
+ StringRef Dir = "path/to/dummy/dir";
+ StringRef Target = "path/to/dummy/dir/target";
+ StringRef Content = "content of target";
+ EXPECT_TRUE(FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(Content)));
+ EXPECT_FALSE(FS.addHardLink(Dir, Target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkUnderAFile) {
+ StringRef CommonContent = "content string";
+
[PATCH] D50515: Re-push "[Option] Fix PR37006 prefix choice in findNearest"
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341329: Re-push [Option] Fix PR37006 prefix choice in
findNearest (authored by modocache, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50515?vs=159931=163737#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50515
Files:
llvm/trunk/lib/Option/OptTable.cpp
llvm/trunk/unittests/Option/OptionParsingTest.cpp
llvm/trunk/unittests/Option/Opts.td
Index: llvm/trunk/lib/Option/OptTable.cpp
===
--- llvm/trunk/lib/Option/OptTable.cpp
+++ llvm/trunk/lib/Option/OptTable.cpp
@@ -252,38 +252,33 @@
unsigned MinimumLength) const {
assert(!Option.empty());
- // Consider each option as a candidate, finding the closest match.
+ // Consider each [option prefix + option name] pair as a candidate, finding
+ // the closest match.
unsigned BestDistance = UINT_MAX;
for (const Info :
ArrayRef(OptionInfos).drop_front(FirstSearchableIndex)) {
StringRef CandidateName = CandidateInfo.Name;
-// Ignore option candidates with empty names, such as "--", or names
-// that do not meet the minimum length.
+// We can eliminate some option prefix/name pairs as candidates right away:
+// * Ignore option candidates with empty names, such as "--", or names
+// that do not meet the minimum length.
if (CandidateName.empty() || CandidateName.size() < MinimumLength)
continue;
-// If FlagsToInclude were specified, ignore options that don't include
-// those flags.
+// * If FlagsToInclude were specified, ignore options that don't include
+// those flags.
if (FlagsToInclude && !(CandidateInfo.Flags & FlagsToInclude))
continue;
-// Ignore options that contain the FlagsToExclude.
+// * Ignore options that contain the FlagsToExclude.
if (CandidateInfo.Flags & FlagsToExclude)
continue;
-// Ignore positional argument option candidates (which do not
-// have prefixes).
+// * Ignore positional argument option candidates (which do not
+// have prefixes).
if (!CandidateInfo.Prefixes)
continue;
-// Find the most appropriate prefix. For example, if a user asks for
-// "--helm", suggest "--help" over "-help".
-StringRef Prefix = CandidateInfo.Prefixes[0];
-for (int P = 1; CandidateInfo.Prefixes[P]; P++) {
- if (Option.startswith(CandidateInfo.Prefixes[P]))
-Prefix = CandidateInfo.Prefixes[P];
-}
-// Check if the candidate ends with a character commonly used when
+// Now check if the candidate ends with a character commonly used when
// delimiting an option from its value, such as '=' or ':'. If it does,
// attempt to split the given option based on that delimiter.
std::string Delimiter = "";
@@ -297,14 +292,19 @@
else
std::tie(LHS, RHS) = Option.split(Last);
-std::string NormalizedName =
-(LHS.drop_front(Prefix.size()) + Delimiter).str();
-unsigned Distance =
-CandidateName.edit_distance(NormalizedName, /*AllowReplacements=*/true,
-/*MaxEditDistance=*/BestDistance);
-if (Distance < BestDistance) {
- BestDistance = Distance;
- NearestString = (Prefix + CandidateName + RHS).str();
+// Consider each possible prefix for each candidate to find the most
+// appropriate one. For example, if a user asks for "--helm", suggest
+// "--help" over "-help".
+for (int P = 0; const char *const CandidatePrefix = CandidateInfo.Prefixes[P]; P++) {
+ std::string NormalizedName = (LHS + Delimiter).str();
+ StringRef Candidate = (CandidatePrefix + CandidateName).str();
+ unsigned Distance =
+ Candidate.edit_distance(NormalizedName, /*AllowReplacements=*/true,
+ /*MaxEditDistance=*/BestDistance);
+ if (Distance < BestDistance) {
+BestDistance = Distance;
+NearestString = (Candidate + RHS).str();
+ }
}
}
return BestDistance;
Index: llvm/trunk/unittests/Option/OptionParsingTest.cpp
===
--- llvm/trunk/unittests/Option/OptionParsingTest.cpp
+++ llvm/trunk/unittests/Option/OptionParsingTest.cpp
@@ -283,6 +283,10 @@
EXPECT_EQ(Nearest, "-blorp");
EXPECT_EQ(1U, T.findNearest("--blorm", Nearest));
EXPECT_EQ(Nearest, "--blorp");
+ EXPECT_EQ(1U, T.findNearest("-blarg", Nearest));
+ EXPECT_EQ(Nearest, "-blarn");
+ EXPECT_EQ(1U, T.findNearest("--blarm", Nearest));
+ EXPECT_EQ(Nearest, "--blarn");
EXPECT_EQ(1U, T.findNearest("-fjormp", Nearest));
EXPECT_EQ(Nearest, "--fjormp");
Index: llvm/trunk/unittests/Option/Opts.td
===
--- llvm/trunk/unittests/Option/Opts.td
+++
[PATCH] D32860: [Analyzer] Iterator Checker - Part 6: Mismatched iterator checker for constructors and comparisons
baloghadamsoftware updated this revision to Diff 163736.
baloghadamsoftware added a comment.
Herald added subscribers: Szelethus, mikhail.ramalho.
Checking of constructor parameters moved to `PreCall` check.
https://reviews.llvm.org/D32860
Files:
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
test/Analysis/mismatched-iterator.cpp
Index: test/Analysis/mismatched-iterator.cpp
===
--- test/Analysis/mismatched-iterator.cpp
+++ test/Analysis/mismatched-iterator.cpp
@@ -3,6 +3,10 @@
#include "Inputs/system-header-simulator-cxx.h"
+void good_ctor(std::vector ) {
+ std::vector new_v(v.cbegin(), v.cend()); // no-warning
+}
+
void good_find(std::vector , int n) {
std::find(v.cbegin(), v.cend(), n); // no-warning
}
@@ -34,6 +38,14 @@
std::find(v2.cbegin(), i0, n); // no-warning
}
+void good_comparison(std::vector ) {
+ if (v.cbegin() == v.cend()) {} // no-warning
+}
+
+void bad_ctor(std::vector , std::vector ) {
+ std::vector new_v(v1.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
void bad_find(std::vector , std::vector , int n) {
std::find(v1.cbegin(), v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
@@ -60,3 +72,9 @@
v1 = std::move(v2);
std::find(v1.cbegin(), i0, n); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
+
+void bad_comparison(std::vector , std::vector ) {
+ if (v1.cbegin() != v2.cend()) { // expected-warning{{Iterators of different containers used where the same container is expected}}
+*v1.cbegin();
+ }
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -236,6 +236,9 @@
void reportMismatchedBug(const StringRef , const SVal ,
const SVal , CheckerContext ,
ExplodedNode *ErrNode) const;
+ void reportMismatchedBug(const StringRef , const SVal ,
+ const MemRegion *Reg, CheckerContext ,
+ ExplodedNode *ErrNode) const;
void reportInvalidatedBug(const StringRef , const SVal ,
CheckerContext , ExplodedNode *ErrNode) const;
@@ -276,6 +279,7 @@
bool isIteratorType(const QualType );
bool isIterator(const CXXRecordDecl *CRD);
+bool isComparisonOperator(OverloadedOperatorKind OK);
bool isBeginCall(const FunctionDecl *Func);
bool isEndCall(const FunctionDecl *Func);
bool isAssignmentOperator(OverloadedOperatorKind OK);
@@ -397,8 +401,48 @@
} else {
verifyDereference(C, Call.getArgSVal(0));
}
+} else if (ChecksEnabled[CK_MismatchedIteratorChecker] &&
+ isComparisonOperator(Func->getOverloadedOperator())) {
+ // Check for comparisons of iterators of different containers
+ if (const auto *InstCall = dyn_cast()) {
+if (Call.getNumArgs() < 1)
+ return;
+
+if (!isIteratorType(InstCall->getCXXThisExpr()->getType()) ||
+!isIteratorType(Call.getArgExpr(0)->getType()))
+ return;
+
+verifyMatch(C, InstCall->getCXXThisVal(), Call.getArgSVal(0));
+ } else {
+if (Call.getNumArgs() < 2)
+ return;
+
+if (!isIteratorType(Call.getArgExpr(0)->getType()) ||
+!isIteratorType(Call.getArgExpr(1)->getType()))
+ return;
+
+verifyMatch(C, Call.getArgSVal(0), Call.getArgSVal(1));
+ }
}
- } else if (!isa()) {
+ } else if (isa()) {
+// Check match of first-last iterator pair in a constructor of a container
+if (Call.getNumArgs() < 2)
+ return;
+
+const auto *Ctr = cast(Call.getDecl());
+if (Ctr->getNumParams() < 2)
+ return;
+
+if (Ctr->getParamDecl(0)->getName() != "first" ||
+Ctr->getParamDecl(1)->getName() != "last")
+ return;
+
+if (!isIteratorType(Call.getArgExpr(0)->getType()) ||
+!isIteratorType(Call.getArgExpr(1)->getType()))
+ return;
+
+verifyMatch(C, Call.getArgSVal(0), Call.getArgSVal(1));
+ } else {
// The main purpose of iterators is to abstract away from different
// containers and provide a (maybe limited) uniform access to them.
// This implies that any correctly written template function that
@@ -1102,6 +1146,16 @@
C.emitReport(std::move(R));
}
+void IteratorChecker::reportMismatchedBug(const StringRef ,
+ const SVal , const MemRegion *Reg,
+ CheckerContext ,
+ ExplodedNode *ErrNode) const {
+ auto R = llvm::make_unique(*MismatchedBugType, Message, ErrNode);
+ R->markInteresting(Val);
+ R->markInteresting(Reg);
+
[PATCH] D50410: Removing -debug-info-macros from option suggestions test
This revision was automatically updated to reflect the committed changes. Closed by commit rL341327: Removing -debug-info-macros from option suggestions test (authored by modocache, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50410?vs=159603=163734#toc Repository: rL LLVM https://reviews.llvm.org/D50410 Files: cfe/trunk/test/Driver/unknown-arg.c Index: cfe/trunk/test/Driver/unknown-arg.c === --- cfe/trunk/test/Driver/unknown-arg.c +++ cfe/trunk/test/Driver/unknown-arg.c @@ -14,7 +14,7 @@ // RUN: FileCheck %s --check-prefix=CL-ERROR-DID-YOU-MEAN // RUN: %clang_cl -cake-is-lie -%0 -%d - -munknown-to-clang-option -print-stats -funknown-to-clang-option -c -Wno-unknown-argument -### -- %s 2>&1 | \ // RUN: FileCheck %s --check-prefix=SILENT -// RUN: not %clang -cc1as -hell --version -debug-info-macros 2>&1 | \ +// RUN: not %clang -cc1as -hell --version 2>&1 | \ // RUN: FileCheck %s --check-prefix=CC1AS-DID-YOU-MEAN // RUN: not %clang -cc1asphalt -help 2>&1 | \ // RUN: FileCheck %s --check-prefix=UNKNOWN-INTEGRATED Index: cfe/trunk/test/Driver/unknown-arg.c === --- cfe/trunk/test/Driver/unknown-arg.c +++ cfe/trunk/test/Driver/unknown-arg.c @@ -14,7 +14,7 @@ // RUN: FileCheck %s --check-prefix=CL-ERROR-DID-YOU-MEAN // RUN: %clang_cl -cake-is-lie -%0 -%d - -munknown-to-clang-option -print-stats -funknown-to-clang-option -c -Wno-unknown-argument -### -- %s 2>&1 | \ // RUN: FileCheck %s --check-prefix=SILENT -// RUN: not %clang -cc1as -hell --version -debug-info-macros 2>&1 | \ +// RUN: not %clang -cc1as -hell --version 2>&1 | \ // RUN: FileCheck %s --check-prefix=CC1AS-DID-YOU-MEAN // RUN: not %clang -cc1asphalt -help 2>&1 | \ // RUN: FileCheck %s --check-prefix=UNKNOWN-INTEGRATED ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341327 - Removing -debug-info-macros from option suggestions test
Author: modocache Date: Mon Sep 3 09:55:02 2018 New Revision: 341327 URL: http://llvm.org/viewvc/llvm-project?rev=341327=rev Log: Removing -debug-info-macros from option suggestions test Summary: https://reviews.llvm.org/D46776 added better support for prefixes for the "did you mean ...?" command line option suggestions. One of the tests was checking against the `-debug-info-macro` option, which was failing on the PS4 build bot. Tests would succeed against the `--help` and `--version` options. From https://llvm.org/devmtg/2013-11/slides/Robinson-PS4Toolchain.pdf, it looks like the PS4 SDK forces optimizations and *could be* disabling the `-debug-info-macro` altogether. This diff removes `-debug-info-macro` altogether. Patch by Arnaud Coomans! Test Plan: untested since we do not have access to a PS4 with the SDK. Reviewers: cfe-commits, modocache Reviewed By: modocache Differential Revision: https://reviews.llvm.org/D50410 Modified: cfe/trunk/test/Driver/unknown-arg.c Modified: cfe/trunk/test/Driver/unknown-arg.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/unknown-arg.c?rev=341327=341326=341327=diff == --- cfe/trunk/test/Driver/unknown-arg.c (original) +++ cfe/trunk/test/Driver/unknown-arg.c Mon Sep 3 09:55:02 2018 @@ -14,7 +14,7 @@ // RUN: FileCheck %s --check-prefix=CL-ERROR-DID-YOU-MEAN // RUN: %clang_cl -cake-is-lie -%0 -%d - -munknown-to-clang-option -print-stats -funknown-to-clang-option -c -Wno-unknown-argument -### -- %s 2>&1 | \ // RUN: FileCheck %s --check-prefix=SILENT -// RUN: not %clang -cc1as -hell --version -debug-info-macros 2>&1 | \ +// RUN: not %clang -cc1as -hell --version 2>&1 | \ // RUN: FileCheck %s --check-prefix=CC1AS-DID-YOU-MEAN // RUN: not %clang -cc1asphalt -help 2>&1 | \ // RUN: FileCheck %s --check-prefix=UNKNOWN-INTEGRATED ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341325: [clangd] Some nitpicking around the new split
(preamble/main) dynamic index (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51221?vs=163730=163732#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51221
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -71,34 +71,57 @@
};
} // namespace
-/// Manages dynamic index for open files. Each file might contribute two sets
-/// of symbols to the dynamic index: symbols from the preamble and symbols
-/// from the file itself. Those have different lifetimes and we merge results
-/// from both
-class ClangdServer::DynamicIndex : public ParsingCallbacks {
+/// The dynamic index tracks symbols visible in open files.
+/// For boring reasons, it doesn't implement SymbolIndex directly - use index().
+class ClangdServer::DynamicIndex {
public:
DynamicIndex(std::vector URISchemes)
: PreambleIdx(URISchemes), MainFileIdx(URISchemes),
MergedIndex(mergeIndex(, )) {}
- SymbolIndex () const { return *MergedIndex; }
+ const SymbolIndex () const { return *MergedIndex; }
- void onPreambleAST(PathRef Path, ASTContext ,
- std::shared_ptr PP) override {
-PreambleIdx.update(Path, , PP, /*TopLevelDecls=*/llvm::None);
- }
-
- void onMainAST(PathRef Path, ParsedAST ) override {
+ // Returns callbacks that can be used to update the index with new ASTs.
+ // Index() presents a merged view of the supplied main-file and preamble ASTs.
+ std::unique_ptr makeUpdateCallbacks() {
+struct CB : public ParsingCallbacks {
+ CB(ClangdServer::DynamicIndex *This) : This(This) {}
+ DynamicIndex *This;
+
+ void onPreambleAST(PathRef Path, ASTContext ,
+ std::shared_ptr PP) override {
+This->PreambleIdx.update(Path, , std::move(PP));
+ }
-MainFileIdx.update(Path, (), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
- }
+ void onMainAST(PathRef Path, ParsedAST ) override {
+This->MainFileIdx.update(Path, (),
+ AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls());
+ }
+};
+return llvm::make_unique(this);
+ };
private:
+ // Contains information from each file's preamble only.
+ // These are large, but update fairly infrequently (preambles are stable).
+ // Missing information:
+ // - symbol occurrences (these are always "from the main file")
+ // - definition locations in the main file
+ //
+ // FIXME: Because the preambles for different TUs have large overlap and
+ // FileIndex doesn't deduplicate, this uses lots of extra RAM.
+ // The biggest obstacle in fixing this: the obvious approach of partitioning
+ // by declaring file (rather than main file) fails if headers provide
+ // different symbols based on preprocessor state.
FileIndex PreambleIdx;
+ // Contains information from each file's main AST.
+ // These are updated frequently (on file change), but are relatively small.
+ // Mostly contains:
+ // - occurrences of symbols declared in the preamble and referenced from main
+ // - symbols declared both in the main file and the preamble
+ // (Note that symbols *only* in the main file are not indexed).
FileIndex MainFileIdx;
- /// Merged view into both indexes. Merges are performed in a similar manner
- /// to the merges of dynamic and static index.
std::unique_ptr MergedIndex;
};
@@ -127,7 +150,7 @@
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
Opts.UpdateDebounce, Opts.RetentionPolicy) {
if (DynamicIdx && Opts.StaticIndex) {
MergedIndex = mergeIndex(>index(), Opts.StaticIndex);
@@ -144,6 +167,10 @@
// ClangdServer::DynamicIndex.
ClangdServer::~ClangdServer() = default;
+const SymbolIndex *ClangdServer::dynamicIndex() const {
+ return
[clang-tools-extra] r341325 - [clangd] Some nitpicking around the new split (preamble/main) dynamic index
Author: sammccall
Date: Mon Sep 3 09:37:59 2018
New Revision: 341325
URL: http://llvm.org/viewvc/llvm-project?rev=341325=rev
Log:
[clangd] Some nitpicking around the new split (preamble/main) dynamic index
Summary:
- DynamicIndex doesn't implement ParsingCallbacks, to make its role clearer.
ParsingCallbacks is a separate object owned by the receiving TUScheduler.
(I tried to get rid of the "index-like-object that doesn't implement index"
but it was too messy).
- Clarified(?) docs around DynamicIndex - fewer details up front, more details
inside.
- Exposed dynamic index from ClangdServer for memory monitoring and more
direct testing of its contents (actual tests not added here, wanted to get
this out for review)
- Removed a redundant and sligthly confusing filename param in a callback
Reviewers: ilya-biryukov
Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet,
cfe-commits
Differential Revision: https://reviews.llvm.org/D51221
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=341325=341324=341325=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Mon Sep 3 09:37:59 2018
@@ -71,34 +71,57 @@ public:
};
} // namespace
-/// Manages dynamic index for open files. Each file might contribute two sets
-/// of symbols to the dynamic index: symbols from the preamble and symbols
-/// from the file itself. Those have different lifetimes and we merge results
-/// from both
-class ClangdServer::DynamicIndex : public ParsingCallbacks {
+/// The dynamic index tracks symbols visible in open files.
+/// For boring reasons, it doesn't implement SymbolIndex directly - use
index().
+class ClangdServer::DynamicIndex {
public:
DynamicIndex(std::vector URISchemes)
: PreambleIdx(URISchemes), MainFileIdx(URISchemes),
MergedIndex(mergeIndex(, )) {}
- SymbolIndex () const { return *MergedIndex; }
+ const SymbolIndex () const { return *MergedIndex; }
- void onPreambleAST(PathRef Path, ASTContext ,
- std::shared_ptr PP) override {
-PreambleIdx.update(Path, , PP, /*TopLevelDecls=*/llvm::None);
- }
-
- void onMainAST(PathRef Path, ParsedAST ) override {
-
-MainFileIdx.update(Path, (), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
- }
+ // Returns callbacks that can be used to update the index with new ASTs.
+ // Index() presents a merged view of the supplied main-file and preamble
ASTs.
+ std::unique_ptr makeUpdateCallbacks() {
+struct CB : public ParsingCallbacks {
+ CB(ClangdServer::DynamicIndex *This) : This(This) {}
+ DynamicIndex *This;
+
+ void onPreambleAST(PathRef Path, ASTContext ,
+ std::shared_ptr PP) override {
+This->PreambleIdx.update(Path, , std::move(PP));
+ }
+
+ void onMainAST(PathRef Path, ParsedAST ) override {
+This->MainFileIdx.update(Path, (),
+ AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls());
+ }
+};
+return llvm::make_unique(this);
+ };
private:
+ // Contains information from each file's preamble only.
+ // These are large, but update fairly infrequently (preambles are stable).
+ // Missing information:
+ // - symbol occurrences (these are always "from the main file")
+ // - definition locations in the main file
+ //
+ // FIXME: Because the preambles for different TUs have large overlap and
+ // FileIndex doesn't deduplicate, this uses lots of extra RAM.
+ // The biggest obstacle in fixing this: the obvious approach of partitioning
+ // by declaring file (rather than main file) fails if headers provide
+ // different symbols based on preprocessor state.
FileIndex PreambleIdx;
+ // Contains information from each file's main AST.
+ // These are updated frequently (on file change), but are relatively small.
+ // Mostly contains:
+ // - occurrences of symbols declared in the preamble and referenced from
main
+ // - symbols declared both in the main file and the preamble
+ // (Note that symbols *only* in the main file are not indexed).
FileIndex MainFileIdx;
- /// Merged view into both indexes. Merges are performed in
[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index
sammccall updated this revision to Diff 163730.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -101,7 +101,7 @@
Notification Ready;
TUScheduler S(
getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
std::atomic CallbackCount(0);
{
TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
// FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
// Run TUScheduler and collect some stats.
{
TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -258,7 +258,7 @@
Policy.MaxRetainedASTs = 2;
TUScheduler S(
/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
llvm::StringLiteral SourceContents = R"cpp(
@@ -308,7 +308,7 @@
TEST_F(TUSchedulerTests, EmptyPreamble) {
TUScheduler S(
/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -355,7 +355,7 @@
// the same time. All reads should get the same non-null preamble.
TUScheduler S(
/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
@@ -388,7 +388,7 @@
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
TUScheduler S(
/*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
- /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -441,7 +441,7 @@
TEST_F(TUSchedulerTests, NoChangeDiags) {
TUScheduler S(
/*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
- /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
buildPreamble(
FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
std::make_shared(), /*StoreInMemory=*/true,
- [, ](PathRef FilePath, ASTContext ,
- std::shared_ptr PP) {
+ [&](ASTContext , std::shared_ptr PP) {
EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
IndexUpdated = true;
-Index.update(FilePath, , std::move(PP));
+Index.update(FooCpp, , std::move(PP));
});
ASSERT_TRUE(IndexUpdated);
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -74,8 +74,6 @@
virtual void onMainAST(PathRef Path, ParsedAST ) {}
};
-ParsingCallbacks ();
-
///
[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:152
WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
Opts.UpdateDebounce, Opts.RetentionPolicy) {
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Any reason to prefer `nullptr` over the no-op callbacks?
> > > Might be a personal preference, my reasoning is: having an extra check
> > > for null before on each of the call sites both adds unnecessary
> > > boilerplate and adds an extra branch before the virtual call (which is,
> > > technically, another form of a branch).
> > >
> > > Not opposed to doing it either way, though.
> > Basically I prefer the old behavior here (when it was std::function).
> > Having to keep the callbacks object alive is a pain, and the shared
> > instance of the no-op implementation for this purpose seems a little silly.
> >
> > We don't have the std::function copyability stuff which is a mixed bag but
> > nice for cases like this. But passing the reference from TUScheduler to its
> > ASTWorkers is internal enough that it doesn't bother me, WDYT?
> >
> > > extra check for null before on each of the call sites
> > Note that the check is actually in the constructor, supporting `nullptr` is
> > just for brevity (particularly in tests).
> > Having to keep the callbacks object alive is a pain, and the shared
> > instance of the no-op implementation for this purpose seems a little silly.
> OTOH, this gives flexibility to the callers wrt to the lifetime, i.e. they
> don't **have** to create a separate object for the callbacks if they don't
> want to (one example of this pattern is ClangdLSPServer inheriting
> DiagnosticsConsumer and ProtocolCallbacks).
>
> But happy to do it either way.
>
I don't think there's any flexibility here, callers *can't* in general create
an object (unless they know enough about the lifetimes to store the object
appropriately).
For dependencies of the callback (e.g. lambda ref captures), either way works
well.
For resources owned by the callback itself, the callee knows better than the
caller when they should be freed.
> ClangdLSPServer inheriting DiagnosticsConsumer and ProtocolCallbacks
FWIW, the former seems like an antipattern that should be std::function, to me.
The latter is indeed different - interfaces with many methods no longer feel
like they fit the lightweight callback pattern, to me...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341324 - Add header guards to some headers that are missing them
Author: akirtzidis
Date: Mon Sep 3 09:26:36 2018
New Revision: 341324
URL: http://llvm.org/viewvc/llvm-project?rev=341324=rev
Log:
Add header guards to some headers that are missing them
Modified:
cfe/trunk/include/clang/AST/ODRHash.h
cfe/trunk/lib/CodeGen/MacroPPCallbacks.h
cfe/trunk/unittests/Rename/ClangRenameTest.h
Modified: cfe/trunk/include/clang/AST/ODRHash.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ODRHash.h?rev=341324=341323=341324=diff
==
--- cfe/trunk/include/clang/AST/ODRHash.h (original)
+++ cfe/trunk/include/clang/AST/ODRHash.h Mon Sep 3 09:26:36 2018
@@ -13,6 +13,9 @@
///
//===--===//
+#ifndef LLVM_CLANG_AST_ODRHASH_H
+#define LLVM_CLANG_AST_ODRHASH_H
+
#include "clang/AST/DeclarationName.h"
#include "clang/AST/Type.h"
#include "clang/AST/TemplateBase.h"
@@ -91,3 +94,5 @@ public:
};
} // end namespace clang
+
+#endif
Modified: cfe/trunk/lib/CodeGen/MacroPPCallbacks.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MacroPPCallbacks.h?rev=341324=341323=341324=diff
==
--- cfe/trunk/lib/CodeGen/MacroPPCallbacks.h (original)
+++ cfe/trunk/lib/CodeGen/MacroPPCallbacks.h Mon Sep 3 09:26:36 2018
@@ -11,6 +11,9 @@
//
//===--===//
+#ifndef LLVM_CLANG_LIB_CODEGEN_MACROPPCALLBACKS_H
+#define LLVM_CLANG_LIB_CODEGEN_MACROPPCALLBACKS_H
+
#include "clang/Lex/PPCallbacks.h"
namespace llvm {
@@ -116,3 +119,5 @@ public:
};
} // end namespace clang
+
+#endif
Modified: cfe/trunk/unittests/Rename/ClangRenameTest.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Rename/ClangRenameTest.h?rev=341324=341323=341324=diff
==
--- cfe/trunk/unittests/Rename/ClangRenameTest.h (original)
+++ cfe/trunk/unittests/Rename/ClangRenameTest.h Mon Sep 3 09:26:36 2018
@@ -7,6 +7,9 @@
//
//===--===//
+#ifndef LLVM_CLANG_UNITTESTS_RENAME_CLANGRENAMETEST_H
+#define LLVM_CLANG_UNITTESTS_RENAME_CLANGRENAMETEST_H
+
#include "unittests/Tooling/RewriterTestContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Basic/FileManager.h"
@@ -110,3 +113,5 @@ protected:
} // namespace test
} // namespace clang_rename
} // namesdpace clang
+
+#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50515: Re-push "[Option] Fix PR37006 prefix choice in findNearest"
modocache accepted this revision. modocache added a comment. This revision is now accepted and ready to land. Thanks! I'll land https://reviews.llvm.org/D50410, then this, and monitor the buildbots to see what happens. If they fail again I may revert this once again. Thanks again for looking into this, sorry for the delay in landing it. Repository: rC Clang https://reviews.llvm.org/D50515 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51321: [Tooling] Improve handling of CL-style options
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks again for fixing this.
Still a few places I feel the code could be simpler, but will let you decide
how to deal with them.
I would greatly appreciate removing the parameterized tests, as that's the
place where I feel least confident I can understand exactly what the intended
behavior is.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:141
+{
+ SmallVector TmpArgv;
+ TmpArgv.reserve(OldArgs.size());
please remove premature optimizations (SmallVector, reserve) - this function is
not (any more) performance-critical
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:143
+ TmpArgv.reserve(OldArgs.size());
+ llvm::transform(OldArgs, std::back_inserter(TmpArgv),
+ [](const std::string ) { return S.c_str(); });
simple loop is more readable than transform()
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:192
+const auto DriverModeName =
+ OptTable.getOption(driver::options::OPT_driver_mode).getPrefixedName();
+
can we just inline this ("--driver-mode") like we do with other specific we
need in their string form (std etc)?
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:196
+for (StringRef S : llvm::reverse(CmdLine)) {
+ if (S.startswith(DriverModeName))
+return S.drop_front(DriverModeName.length()) == "cl";
```
if (S.consume_front(...))
return S == "cl";
```
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector ,
+ const llvm::opt::OptTable ) {
hamzasood wrote:
> hamzasood wrote:
> > sammccall wrote:
> > > nit: a two-value enum would be clearer and allow for terser variable
> > > names (`Mode`)
> > The advantage of a Boolean is that it makes the checks simpler. E.g. `if
> > (isCL)` instead of `if (mode == DriverMode::CL)` or something.
> >
> > Happy to change it though if you still disagree.
> Also, there're more than just 2 driver modes. But we only care about cl and
> not cl.
I do find `if (mode == DriverMode::CL)` much clearer.
"CL" isn't a particularly clear name for people who don't deal with windows
much, and "!isCL" isn't a great name for the GCC-compatible driver.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156
+
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as
the
hamzasood wrote:
> hamzasood wrote:
> > sammccall wrote:
> > > would you mind reverting this change and just wrapping the old Argv in an
> > > InputArgList?
> > > I'm not sure the lifetime comments and std::transform aid readability
> > > here.
> > The change was more about safety than readability. The old approach left an
> > InputArgList of dangling pointers after moving the new args into the cmd
> > object. This way there’s no way to accidentally access deallocated memory
> > by using the InputArgList.
> >
> > As above, happy to change if you still disagree.
> I've restructured it so hopefully this is less of a concern.
I think a comment `// ArgList is no longer valid` would suffice, or storing the
ArgList in an `Optional` and resetting it.
In principle the restructuring seems fine, but it introduced new issues: the
boundaries and data flow between the constructor and `processInputCommandLine`
is unclear. (It reads from parameters which are derived from `Cmd.CommandLine`
and overwrites `Cmd.CommandLine` itself, along with other state. Its name
doesn't help clarify what its responsibility is.
If you want to keep this function separate, I'd suggest:
- make it static to avoid confusion about state while the constructor is
running
- call it `parseCommandLine` to reflect the processing it's doing
- return `tuple, DriverMode, LangStandard, Optional>`
- call it as `std::tie(Cmd.CommandLine, Mode, ...) = parseCommandLine(...)`
But it also seems fine as part of the constructor.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:231
: Type;
- Result.CommandLine.push_back("-x");
- Result.CommandLine.push_back(types::getTypeName(TargetType));
+ if (IsCLMode) {
+const StringRef Flag = toCLFlag(TargetType);
hamzasood wrote:
> sammccall wrote:
> > can we unify as `addTypeFlag(TargetType, Mode, Result.CommandLine)`?
> To clarify, you mean extract this into a helper function?
Right - you already have the helper function `toCLflag`, I'd suggest
extending/renaming it so it covers both CL and GCC and fits the call site more
conveniently.
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
ilya-biryukov added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:478
+public:
+ InMemoryNode(const std::string& FileName, InMemoryNodeKind Kind)
+ : Kind(Kind), FileName(llvm::sys::path::filename(FileName.data())) {}
NIT: accept a parameter of type `llvm::StringRef` instead of `const
std::string&`.
Comment at: lib/Basic/VirtualFileSystem.cpp:495
+ InMemoryFile(Status Stat, std::unique_ptr Buffer)
+ : InMemoryNode(Stat.getName().str(), IME_File), Stat(std::move(Stat)),
+Buffer(std::move(Buffer)) {}
NIT: `.str()` is redundant (in fact, a pessimization) if parameter of
`InMemoryNode` is a `StringRef`.
Comment at: lib/Basic/VirtualFileSystem.cpp:522
+ InMemoryHardLink(StringRef Path, const InMemoryFile )
+ : InMemoryNode(Path.str(), IME_HardLink), ResolvedFile(ResolvedFile) {}
+ const InMemoryFile () const { return ResolvedFile; }
NIT: as noted in the other comment, `.str()` can be removed if parameter type
is changed.
Comment at: lib/Basic/VirtualFileSystem.cpp:570
InMemoryDirectory(Status Stat)
- : InMemoryNode(std::move(Stat), IME_Directory) {}
+ : InMemoryNode(Stat.getName().str(), IME_Directory),
+Stat(std::move(Stat)) {}
NIT: `.str()` can be removed
Comment at: lib/Basic/VirtualFileSystem.cpp:662
const auto ResolvedPerms = Perms.getValueOr(sys::fs::all_all);
+ // Cannot create HardLink from a directory.
+ if (HardLinkTarget && Buffer)
The comment looks outdated
Comment at: lib/Basic/VirtualFileSystem.cpp:663
+ // Cannot create HardLink from a directory.
+ if (HardLinkTarget && Buffer)
+return false;
Shouldn't we assert this instead? This is not an invalid input, this is
actually breaking our program logic, right? I.e. one shouldn't call addFile
with HardLinkTarget set and non-null buffer
Comment at: lib/Basic/VirtualFileSystem.cpp:677
+if (HardLinkTarget)
+ Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget));
+else {
NIT: `.str()` seems redundant
Comment at: lib/Basic/VirtualFileSystem.cpp:804
+ return this->addFile(FromPath, 0, nullptr, None, None, None, None,
+ static_cast(*ToNode));
+}
NIT: use `llvm::cast` instead of static_cast.
It's essentially equivalent with added asserts.
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:974
+TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) {
+ Twine FromLink = "/path/to/FROM/link";
+ Twine Target = "/path/to/TO/file";
Please use `StringRef`s everywhere. `Twine` should only be generally only be
used as temporary objects, see the specific guidelines in Twine's docs.
Whenever a reference to an existing string is needed, a `StringRef` is the best
choice.
If you need to create new strings (e.g. concatenation, etc.), use `std::string`.
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:976
+ Twine Target = "/path/to/TO/file";
+ StringRef content = "content of target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(content));
s/content/Content
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:987
+ StringRef content = "content of target file";
+ Twine link0 = "/path/to/0/link";
+ Twine link1 = "/path/to/1/link";
s/link0/Link0
and other `lowerCamelCase` vars --> `UpperCamelCase`
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:1090
+ }
+ EXPECT_EQ(1, Counts[0]); // a
+ EXPECT_EQ(1, Counts[1]); // b
This seems equivalent to `EXPECT_THAT(Nodes, UnorderedElementsAre("/a", "/a/b",
"/c", "/c/d")`. Maybe simplify?
Note that iterators might give different file separators on windows. You might
want to normalize, e.g. with `llvm::sys::path::native(..., Style::posix)`.
And still watch out for windows buildbot failures, in case the root (/) is
interpreted differently on Windows, possibly giving different results.
Repository:
rC Clang
https://reviews.llvm.org/D51359
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51416: [RTTI] Align rtti types to prevent over-alignment
dmgreen updated this revision to Diff 163727.
dmgreen retitled this revision from "[RTTI] Align rtti type string to prevent
over-alignment" to "[RTTI] Align rtti types to prevent over-alignment".
dmgreen edited the summary of this revision.
dmgreen added a comment.
I've become less sure about what the various alignments should be here. There
seem to be multiple ways to calculate such things, I'm not sure which are best.
For example, for want of a better option I've used getPointerAlign in
ItaniumRTTIBuilder::BuildTypeInfo.
https://reviews.llvm.org/D51416
Files:
lib/CodeGen/CGVTT.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/ItaniumCXXABI.cpp
lib/CodeGen/MicrosoftCXXABI.cpp
test/CodeGenCXX/vtable-align.cpp
test/CodeGenCXX/vtable-linkage.cpp
Index: test/CodeGenCXX/vtable-linkage.cpp
===
--- test/CodeGenCXX/vtable-linkage.cpp
+++ test/CodeGenCXX/vtable-linkage.cpp
@@ -98,10 +98,10 @@
// C has no key function, so its vtable should have weak_odr linkage
// and hidden visibility (rdar://problem/7523229).
-// CHECK-DAG: @_ZTV1C = linkonce_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1C = linkonce_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1C = linkonce_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTT1C = linkonce_odr unnamed_addr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTV1C = linkonce_odr unnamed_addr constant {{.*}}, comdat, align 8{{$}}
+// CHECK-DAG: @_ZTS1C = linkonce_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1C = linkonce_odr constant {{.*}}, comdat, align 8{{$}}
+// CHECK-DAG: @_ZTT1C = linkonce_odr unnamed_addr constant {{.*}}, comdat, align 8{{$}}
// D has a key function that is defined in this translation unit so its vtable is
// defined in the translation unit.
@@ -120,27 +120,27 @@
// defined in this translation unit, so its vtable should have
// weak_odr linkage.
// CHECK-DAG: @_ZTV1EIsE = weak_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1EIsE = weak_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1EIsE = weak_odr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTS1EIsE = weak_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1EIsE = weak_odr constant {{.*}}, comdat, align 8{{$}}
// F is an explicit template instantiation without a key
// function, so its vtable should have weak_odr linkage
// CHECK-DAG: @_ZTV1FIsE = weak_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1FIsE = weak_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1FIsE = weak_odr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTS1FIsE = weak_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1FIsE = weak_odr constant {{.*}}, comdat, align 8{{$}}
// E is an implicit template instantiation with a key function
// defined in this translation unit, so its vtable should have
// linkonce_odr linkage.
// CHECK-DAG: @_ZTV1EIlE = linkonce_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1EIlE = linkonce_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1EIlE = linkonce_odr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTS1EIlE = linkonce_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1EIlE = linkonce_odr constant {{.*}}, comdat, align 8{{$}}
// F is an implicit template instantiation with no key function,
// so its vtable should have linkonce_odr linkage.
// CHECK-DAG: @_ZTV1FIlE = linkonce_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1FIlE = linkonce_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1FIlE = linkonce_odr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTS1FIlE = linkonce_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1FIlE = linkonce_odr constant {{.*}}, comdat, align 8{{$}}
// F is an explicit template instantiation declaration without a
// key function, so its vtable should have external linkage.
@@ -171,8 +171,8 @@
// F is an explicit specialization without a key function, so
// its vtable should have linkonce_odr linkage.
// CHECK-DAG: @_ZTV1FIcE = linkonce_odr unnamed_addr constant {{.*}}, comdat,
-// CHECK-DAG: @_ZTS1FIcE = linkonce_odr constant {{.*}}, comdat{{$}}
-// CHECK-DAG: @_ZTI1FIcE = linkonce_odr constant {{.*}}, comdat{{$}}
+// CHECK-DAG: @_ZTS1FIcE = linkonce_odr constant {{.*}}, comdat, align 1{{$}}
+// CHECK-DAG: @_ZTI1FIcE = linkonce_odr constant {{.*}}, comdat, align 8{{$}}
// CHECK-DAG: @_ZTV1GIiE = linkonce_odr unnamed_addr constant {{.*}}, comdat,
template
Index: test/CodeGenCXX/vtable-align.cpp
===
--- test/CodeGenCXX/vtable-align.cpp
+++ test/CodeGenCXX/vtable-align.cpp
@@ -10,5 +10,8 @@
void A::f() {}
// CHECK-32: @_ZTV1A = unnamed_addr constant { [5 x i8*] } { [5 x i8*] [i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI1A to i8*), i8* bitcast (void (%struct.A*)* @_ZN1A1fEv to
[PATCH] D32859: [Analyzer] Iterator Checker - Part 5: Move Assignment of Containers
baloghadamsoftware updated this revision to Diff 163725.
baloghadamsoftware added a comment.
) added.
https://reviews.llvm.org/D32859
Files:
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
test/Analysis/mismatched-iterator.cpp
Index: test/Analysis/mismatched-iterator.cpp
===
--- test/Analysis/mismatched-iterator.cpp
+++ test/Analysis/mismatched-iterator.cpp
@@ -15,11 +15,48 @@
std::copy(v1.cbegin(), v1.cend(), v2.begin()); // no-warning
}
+void good_move_find1(std::vector , std::vector , int n) {
+ auto i0 = v2.cbegin();
+ v1 = std::move(v2);
+ std::find(i0, v1.cend(), n); // no-warning
+}
+
+void good_move_find2(std::vector , std::vector , int n) {
+ auto i0 = --v2.cend();
+ v1 = std::move(v2);
+ std::find(i0, v1.cend(), n); // no-warning
+}
+
+void good_move_find3(std::vector , std::vector , int n) {
+ auto i0 = v2.cend();
+ v1 = std::move(v2);
+ v2.push_back(n);
+ std::find(v2.cbegin(), i0, n); // no-warning
+}
+
void bad_find(std::vector , std::vector , int n) {
std::find(v1.cbegin(), v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
void bad_find_first_of(std::vector , std::vector ) {
std::find_first_of(v1.cbegin(), v2.cend(), v2.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
std::find_first_of(v1.cbegin(), v1.cend(), v2.cbegin(), v1.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
+
+void bad_move_find1(std::vector , std::vector , int n) {
+ auto i0 = v2.cbegin();
+ v1 = std::move(v2);
+ std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
+void bad_move_find2(std::vector , std::vector , int n) {
+ auto i0 = --v2.cend();
+ v1 = std::move(v2);
+ std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
+void bad_move_find3(std::vector , std::vector , int n) {
+ auto i0 = v2.cend();
+ v1 = std::move(v2);
+ std::find(v1.cbegin(), i0, n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -114,6 +114,10 @@
return IteratorPosition(Cont, Valid, NewOf);
}
+ IteratorPosition reAssign(const MemRegion *NewCont) const {
+return IteratorPosition(NewCont, Valid, Offset);
+ }
+
bool operator==(const IteratorPosition ) const {
return Cont == X.Cont && Valid == X.Valid && Offset == X.Offset;
}
@@ -218,7 +222,9 @@
const SVal ) const;
void assignToContainer(CheckerContext , const Expr *CE, const SVal ,
const MemRegion *Cont) const;
- void handleAssign(CheckerContext , const SVal ) const;
+ void handleAssign(CheckerContext , const SVal ,
+const Expr *CE = nullptr,
+const SVal = UndefinedVal()) const;
void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
const SVal , const SVal ,
const SVal ) const;
@@ -315,6 +321,17 @@
bool Equal);
ProgramStateRef invalidateAllIteratorPositions(ProgramStateRef State,
const MemRegion *Cont);
+ProgramStateRef reassignAllIteratorPositions(ProgramStateRef State,
+ const MemRegion *Cont,
+ const MemRegion *NewCont);
+ProgramStateRef reassignAllIteratorPositionsUnless(ProgramStateRef State,
+ const MemRegion *Cont,
+ const MemRegion *NewCont,
+ SymbolRef Offset,
+ BinaryOperator::Opcode Opc);
+ProgramStateRef rebaseSymbolInIteratorPositionsIf(
+ProgramStateRef State, SValBuilder , SymbolRef OldSym,
+SymbolRef NewSym, SymbolRef CondSym, BinaryOperator::Opcode Opc);
const ContainerData *getContainerData(ProgramStateRef State,
const MemRegion *Cont);
ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont,
@@ -454,7 +471,12 @@
const auto Op = Func->getOverloadedOperator();
if (isAssignmentOperator(Op)) {
const auto *InstCall = dyn_cast();
- handleAssign(C, InstCall->getCXXThisVal());
+ if (Func->getParamDecl(0)->getType()->isRValueReferenceType()) {
+handleAssign(C,
[PATCH] D32859: [Analyzer] Iterator Checker - Part 5: Move Assignment of Containers
baloghadamsoftware updated this revision to Diff 163724.
baloghadamsoftware added a comment.
Comments added, functions renamed.
https://reviews.llvm.org/D32859
Files:
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
test/Analysis/mismatched-iterator.cpp
Index: test/Analysis/mismatched-iterator.cpp
===
--- test/Analysis/mismatched-iterator.cpp
+++ test/Analysis/mismatched-iterator.cpp
@@ -15,11 +15,48 @@
std::copy(v1.cbegin(), v1.cend(), v2.begin()); // no-warning
}
+void good_move_find1(std::vector , std::vector , int n) {
+ auto i0 = v2.cbegin();
+ v1 = std::move(v2);
+ std::find(i0, v1.cend(), n); // no-warning
+}
+
+void good_move_find2(std::vector , std::vector , int n) {
+ auto i0 = --v2.cend();
+ v1 = std::move(v2);
+ std::find(i0, v1.cend(), n); // no-warning
+}
+
+void good_move_find3(std::vector , std::vector , int n) {
+ auto i0 = v2.cend();
+ v1 = std::move(v2);
+ v2.push_back(n);
+ std::find(v2.cbegin(), i0, n); // no-warning
+}
+
void bad_find(std::vector , std::vector , int n) {
std::find(v1.cbegin(), v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
void bad_find_first_of(std::vector , std::vector ) {
std::find_first_of(v1.cbegin(), v2.cend(), v2.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
std::find_first_of(v1.cbegin(), v1.cend(), v2.cbegin(), v1.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
}
+
+void bad_move_find1(std::vector , std::vector , int n) {
+ auto i0 = v2.cbegin();
+ v1 = std::move(v2);
+ std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
+void bad_move_find2(std::vector , std::vector , int n) {
+ auto i0 = --v2.cend();
+ v1 = std::move(v2);
+ std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
+void bad_move_find3(std::vector , std::vector , int n) {
+ auto i0 = v2.cend();
+ v1 = std::move(v2);
+ std::find(v1.cbegin(), i0, n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -114,6 +114,10 @@
return IteratorPosition(Cont, Valid, NewOf);
}
+ IteratorPosition reAssign(const MemRegion *NewCont) const {
+return IteratorPosition(NewCont, Valid, Offset);
+ }
+
bool operator==(const IteratorPosition ) const {
return Cont == X.Cont && Valid == X.Valid && Offset == X.Offset;
}
@@ -218,7 +222,9 @@
const SVal ) const;
void assignToContainer(CheckerContext , const Expr *CE, const SVal ,
const MemRegion *Cont) const;
- void handleAssign(CheckerContext , const SVal ) const;
+ void handleAssign(CheckerContext , const SVal ,
+const Expr *CE = nullptr,
+const SVal = UndefinedVal()) const;
void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
const SVal , const SVal ,
const SVal ) const;
@@ -315,6 +321,17 @@
bool Equal);
ProgramStateRef invalidateAllIteratorPositions(ProgramStateRef State,
const MemRegion *Cont);
+ProgramStateRef reassignAllIteratorPositions(ProgramStateRef State,
+ const MemRegion *Cont,
+ const MemRegion *NewCont);
+ProgramStateRef reassignAllIteratorPositionsUnless(ProgramStateRef State,
+ const MemRegion *Cont,
+ const MemRegion *NewCont,
+ SymbolRef Offset,
+ BinaryOperator::Opcode Opc);
+ProgramStateRef rebaseSymbolInIteratorPositionsIf(
+ProgramStateRef State, SValBuilder , SymbolRef OldSym,
+SymbolRef NewSym, SymbolRef CondSym, BinaryOperator::Opcode Opc);
const ContainerData *getContainerData(ProgramStateRef State,
const MemRegion *Cont);
ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont,
@@ -454,7 +471,12 @@
const auto Op = Func->getOverloadedOperator();
if (isAssignmentOperator(Op)) {
const auto *InstCall = dyn_cast();
- handleAssign(C, InstCall->getCXXThisVal());
+ if (Func->getParamDecl(0)->getType()->isRValueReferenceType()) {
+
[PATCH] D50958: [clangd] Implement findReferences function
sammccall added a comment.
Looking forward to using this!
Unless you object, I'd like to address the remaining comments (and get a
review), so you can make the most of your leave!
Comment at: clangd/XRefs.cpp:333
+
+DeclOccurrences[D].emplace_back(FileLoc, Roles);
+return true;
why is this a map keyed by D? the keys are never used, the result is always
flattened into a vector.
Comment at: clangd/XRefs.cpp:361
+/// Find symbol occurrences that a given whilelist of declarations refers to.
+class OccurrencesFinder : public OccurrenceCollector {
+public:
why is this a subclass rather than just using OccurrenceCollector and putting
the finish() code in findReferences()?
Comment at: clangd/XRefs.cpp:378
+}
+// Deduplicate results.
+std::sort(Occurrences.begin(), Occurrences.end());
out of curiosity: how do we actually get duplicates?
Comment at: clangd/XRefs.cpp:388
+
+/// Finds document highlights that a given whitelist of declarations refers to.
+class DocumentHighlightsFinder : public OccurrenceCollector {
as above, why does this need to be a subclass
Comment at: clangd/XRefs.cpp:718
+ getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+ // Identified symbols at a specific position.
+ auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
(comment just echoes code)
Comment at: clangd/XRefs.cpp:721
+
+ // For local symbols (e.g. symbols that are only visiable in main file,
+ // symbols defined in function body), we can get complete references by
visiable -> visible
Comment at: clangd/XRefs.cpp:724
+ // traversing the AST, and we don't want to make unnecessary queries to the
+ // index. Howerver, we don't have a reliable way to distinguish file-local
+ // symbols. We conservatively consider function-local symbols.
we can check isExternallyVisible, I think?
maybe it's not worth it, but the comment as it stands doesn't seem accurate
Comment at: unittests/clangd/XRefsTests.cpp:1193
+ )";
+ MockIndex Index;
+ for (const char *Test : Tests) {
can we use a simple real implementation `TU.index()` instead of mocking here?
I think this is an artifact of the fact that TestTU doesn't actually expose
occurrences in the index, can we fix that?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341322: [clangd] Avoid crashes in override completions
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D51598
Files:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1765,6 +1765,21 @@
Not(Contains(Labeled("void vfunc(bool param)
override");
}
+TEST(CompletionTest, OverridesNonIdentName) {
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
+ virtual ~Base() = 0;
+ virtual operator int() = 0;
+ virtual Base& operator+(Base&) = 0;
+};
+
+struct Derived : Base {
+ ^
+};
+ )cpp");
+}
+
TEST(SpeculateCompletionFilter, Filters) {
Annotations F(R"cpp($bof^
$bol^
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -210,7 +210,7 @@
// These are stored by name to make querying fast in the later step.
llvm::StringMap> Overrides;
for (auto *Method : CR->methods()) {
-if (!Method->isVirtual())
+if (!Method->isVirtual() || !Method->getIdentifier())
continue;
Overrides[Method->getName()].push_back(Method);
}
@@ -221,14 +221,14 @@
if (!BR)
continue;
for (auto *Method : BR->methods()) {
- if (!Method->isVirtual())
+ if (!Method->isVirtual() || !Method->getIdentifier())
continue;
const auto it = Overrides.find(Method->getName());
bool IsOverriden = false;
if (it != Overrides.end()) {
for (auto *MD : it->second) {
// If the method in current body is not an overload of this virtual
- // function, that it overrides this one.
+ // function, then it overrides this one.
if (!S->IsOverload(MD, Method, false)) {
IsOverriden = true;
break;
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1765,6 +1765,21 @@
Not(Contains(Labeled("void vfunc(bool param) override");
}
+TEST(CompletionTest, OverridesNonIdentName) {
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
+ virtual ~Base() = 0;
+ virtual operator int() = 0;
+ virtual Base& operator+(Base&) = 0;
+};
+
+struct Derived : Base {
+ ^
+};
+ )cpp");
+}
+
TEST(SpeculateCompletionFilter, Filters) {
Annotations F(R"cpp($bof^
$bol^
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -210,7 +210,7 @@
// These are stored by name to make querying fast in the later step.
llvm::StringMap> Overrides;
for (auto *Method : CR->methods()) {
-if (!Method->isVirtual())
+if (!Method->isVirtual() || !Method->getIdentifier())
continue;
Overrides[Method->getName()].push_back(Method);
}
@@ -221,14 +221,14 @@
if (!BR)
continue;
for (auto *Method : BR->methods()) {
- if (!Method->isVirtual())
+ if (!Method->isVirtual() || !Method->getIdentifier())
continue;
const auto it = Overrides.find(Method->getName());
bool IsOverriden = false;
if (it != Overrides.end()) {
for (auto *MD : it->second) {
// If the method in current body is not an overload of this virtual
- // function, that it overrides this one.
+ // function, then it overrides this one.
if (!S->IsOverload(MD, Method, false)) {
IsOverriden = true;
break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r341322 - [clangd] Avoid crashes in override completions
Author: ibiryukov
Date: Mon Sep 3 08:25:27 2018
New Revision: 341322
URL: http://llvm.org/viewvc/llvm-project?rev=341322=rev
Log:
[clangd] Avoid crashes in override completions
Summary: NamedDecl::getName cannot be called on non-identifier names.
Reviewers: kadircet, ioeric, hokein, sammccall
Reviewed By: ioeric
Subscribers: MaskRay, jkorous, arphaman, cfe-commits
Differential Revision: https://reviews.llvm.org/D51598
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=341322=341321=341322=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Mon Sep 3 08:25:27 2018
@@ -210,7 +210,7 @@ getNonOverridenMethodCompletionResults(c
// These are stored by name to make querying fast in the later step.
llvm::StringMap> Overrides;
for (auto *Method : CR->methods()) {
-if (!Method->isVirtual())
+if (!Method->isVirtual() || !Method->getIdentifier())
continue;
Overrides[Method->getName()].push_back(Method);
}
@@ -221,14 +221,14 @@ getNonOverridenMethodCompletionResults(c
if (!BR)
continue;
for (auto *Method : BR->methods()) {
- if (!Method->isVirtual())
+ if (!Method->isVirtual() || !Method->getIdentifier())
continue;
const auto it = Overrides.find(Method->getName());
bool IsOverriden = false;
if (it != Overrides.end()) {
for (auto *MD : it->second) {
// If the method in current body is not an overload of this virtual
- // function, that it overrides this one.
+ // function, then it overrides this one.
if (!S->IsOverload(MD, Method, false)) {
IsOverriden = true;
break;
Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=341322=341321=341322=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Mon Sep 3
08:25:27 2018
@@ -1765,6 +1765,21 @@ TEST(CompletionTest, SuggestOverrides) {
Not(Contains(Labeled("void vfunc(bool param)
override");
}
+TEST(CompletionTest, OverridesNonIdentName) {
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
+ virtual ~Base() = 0;
+ virtual operator int() = 0;
+ virtual Base& operator+(Base&) = 0;
+};
+
+struct Derived : Base {
+ ^
+};
+ )cpp");
+}
+
TEST(SpeculateCompletionFilter, Filters) {
Annotations F(R"cpp($bof^
$bol^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r341321 - [clangd] Fix ambiguous make_unique with c++17. NFC
Author: sammccall
Date: Mon Sep 3 08:23:01 2018
New Revision: 341321
URL: http://llvm.org/viewvc/llvm-project?rev=341321=rev
Log:
[clangd] Fix ambiguous make_unique with c++17. NFC
Modified:
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
Modified: clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp?rev=341321=341320=341321=diff
==
--- clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp Mon Sep 3 08:23:01
2018
@@ -56,11 +56,11 @@ TEST(SwapIndexTest, OldIndexRecycled) {
auto Token = std::make_shared();
std::weak_ptr WeakToken = Token;
- SwapIndex S(make_unique(SymbolSlab(), MemIndex::OccurrenceMap(),
-std::move(Token)));
- EXPECT_FALSE(WeakToken.expired()); // Current MemIndex keeps it alive.
- S.reset(make_unique()); // Now the MemIndex is destroyed.
- EXPECT_TRUE(WeakToken.expired()); // So the token is too.
+ SwapIndex S(llvm::make_unique(
+ SymbolSlab(), MemIndex::OccurrenceMap(), std::move(Token)));
+ EXPECT_FALSE(WeakToken.expired()); // Current MemIndex keeps it alive.
+ S.reset(llvm::make_unique()); // Now the MemIndex is destroyed.
+ EXPECT_TRUE(WeakToken.expired()); // So the token is too.
}
TEST(MemIndexTest, MemIndexDeduplicate) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov added inline comments.
Comment at: unittests/clangd/CodeCompleteTests.cpp:1770
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
ioeric wrote:
> ilya-biryukov wrote:
> > Was wondering if testing for crashes LG this way, or adding more assertions
> > might make sense too
> Hmm, I think this is okay, but I'd probably do some sanity check on the
> results, just to make it look less odd ;)
Exactly my feelings: this looks odd.
However, couldn't come up with a decent sanity check at this point.
The reason is: we don't store enough information to tell override completion
from non-override ones.
It means I can assert something like `Not(Contains(Labelled("~Base()
override")))`, but lots of broken outputs can still make the test pass, e.g.:
- `void ~Base() override`
- `~Derived() override`
- ...
Will probably keep as this and think how to factor out overriden completions
from the results better...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51598
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ioeric added inline comments.
Comment at: unittests/clangd/CodeCompleteTests.cpp:1770
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
ilya-biryukov wrote:
> Was wondering if testing for crashes LG this way, or adding more assertions
> might make sense too
Hmm, I think this is okay, but I'd probably do some sanity check on the
results, just to make it look less odd ;)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51598
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall updated this revision to Diff 163722.
sammccall added a comment.
Rebase with occurrences changes (but don't serialize occurrences yet).
Also incorporate multiple-include-header change, which is tricky as it makes
Symbol variable-length. Current hack is to preserve only the most-popular 50
includes, open to other ideas.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51585
Files:
clangd/CMakeLists.txt
clangd/RIFF.cpp
clangd/RIFF.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/RIFFTests.cpp
unittests/clangd/SerializationTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -45,7 +45,6 @@
MATCHER_P(Labeled, Label, "") {
return (arg.Name + arg.Signature).str() == Label;
}
-MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
MATCHER_P(Snippet, S, "") {
@@ -746,84 +745,6 @@
Snippet("ff(${1:int x}, ${2:double y})";
}
-TEST_F(SymbolCollectorTest, YAMLConversions) {
- const std::string YAML1 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
-Name: 'Foo1'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/foo.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:true
-Documentation:'Foo doc'
-ReturnType:'int'
-IncludeHeaders:
- - Header:'include1'
-References:7
- - Header:'include2'
-References:3
-...
-)";
- const std::string YAML2 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
-Name: 'Foo2'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/bar.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:false
-Signature:'-sig'
-CompletionSnippetSuffix:'-snippet'
-...
-)";
-
- auto Symbols1 = symbolsFromYAML(YAML1);
-
- EXPECT_THAT(Symbols1,
- UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
- Doc("Foo doc"), ReturnType("int"),
- DeclURI("file:///path/foo.h"),
- ForCodeCompletion(true;
- auto = *Symbols1.begin();
- EXPECT_THAT(Sym1.IncludeHeaders,
- UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
- IncludeHeaderWithRef("include2", 3u)));
- auto Symbols2 = symbolsFromYAML(YAML2);
- EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
-QName("clang::Foo2"), Labeled("Foo2-sig"),
-Not(HasReturnType()), DeclURI("file:///path/bar.h"),
-ForCodeCompletion(false;
-
- std::string ConcatenatedYAML;
- {
-llvm::raw_string_ostream OS(ConcatenatedYAML);
-SymbolsToYAML(Symbols1, OS);
-SymbolsToYAML(Symbols2, OS);
- }
- auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
- EXPECT_THAT(ConcatenatedSymbols,
- UnorderedElementsAre(QName("clang::Foo1"),
- QName("clang::Foo2")));
-}
-
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
Index: unittests/clangd/SerializationTests.cpp
===
--- /dev/null
+++ unittests/clangd/SerializationTests.cpp
@@ -0,0 +1,138 @@
+//===-- SerializationTests.cpp - Binary and YAML serialization unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/Serialization.h"
+#include "index/SymbolYAML.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
+namespace clang {
+namespace clangd {
+namespace {
+
+const char *YAML1 = R"(
+---
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
+Name: 'Foo1'
+Scope: 'clang::'
+SymInfo:
+ Kind:Function
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI:file:///path/foo.h
+ Start:
+Line: 1
+
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov added inline comments.
Comment at: unittests/clangd/CodeCompleteTests.cpp:1770
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
Was wondering if testing for crashes LG this way, or adding more assertions
might make sense too
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51598
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov created this revision.
ilya-biryukov added reviewers: kadircet, ioeric, hokein, sammccall.
Herald added subscribers: arphaman, jkorous, MaskRay.
NamedDecl::getName cannot be called on non-identifier names.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51598
Files:
clangd/CodeComplete.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1765,6 +1765,21 @@
Not(Contains(Labeled("void vfunc(bool param)
override");
}
+TEST(CompletionTest, OverridesNonIdentName) {
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
+ virtual ~Base() = 0;
+ virtual operator int() = 0;
+ virtual Base& operator+(Base&) = 0;
+};
+
+struct Derived : Base {
+ ^
+};
+ )cpp");
+}
+
TEST(SpeculateCompletionFilter, Filters) {
Annotations F(R"cpp($bof^
$bol^
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -210,7 +210,7 @@
// These are stored by name to make querying fast in the later step.
llvm::StringMap> Overrides;
for (auto *Method : CR->methods()) {
-if (!Method->isVirtual())
+if (!Method->isVirtual() || !Method->getIdentifier())
continue;
Overrides[Method->getName()].push_back(Method);
}
@@ -221,14 +221,14 @@
if (!BR)
continue;
for (auto *Method : BR->methods()) {
- if (!Method->isVirtual())
+ if (!Method->isVirtual() || !Method->getIdentifier())
continue;
const auto it = Overrides.find(Method->getName());
bool IsOverriden = false;
if (it != Overrides.end()) {
for (auto *MD : it->second) {
// If the method in current body is not an overload of this virtual
- // function, that it overrides this one.
+ // function, then it overrides this one.
if (!S->IsOverload(MD, Method, false)) {
IsOverriden = true;
break;
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1765,6 +1765,21 @@
Not(Contains(Labeled("void vfunc(bool param) override");
}
+TEST(CompletionTest, OverridesNonIdentName) {
+ // Check the completions call does not crash.
+ completions(R"cpp(
+struct Base {
+ virtual ~Base() = 0;
+ virtual operator int() = 0;
+ virtual Base& operator+(Base&) = 0;
+};
+
+struct Derived : Base {
+ ^
+};
+ )cpp");
+}
+
TEST(SpeculateCompletionFilter, Filters) {
Annotations F(R"cpp($bof^
$bol^
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -210,7 +210,7 @@
// These are stored by name to make querying fast in the later step.
llvm::StringMap> Overrides;
for (auto *Method : CR->methods()) {
-if (!Method->isVirtual())
+if (!Method->isVirtual() || !Method->getIdentifier())
continue;
Overrides[Method->getName()].push_back(Method);
}
@@ -221,14 +221,14 @@
if (!BR)
continue;
for (auto *Method : BR->methods()) {
- if (!Method->isVirtual())
+ if (!Method->isVirtual() || !Method->getIdentifier())
continue;
const auto it = Overrides.find(Method->getName());
bool IsOverriden = false;
if (it != Overrides.end()) {
for (auto *MD : it->second) {
// If the method in current body is not an overload of this virtual
- // function, that it overrides this one.
+ // function, then it overrides this one.
if (!S->IsOverload(MD, Method, false)) {
IsOverriden = true;
break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:3763 +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods, +DefaultTestValuesForRunOptions, ); This hunk has nothing to do with this change, but previously we forgot to instantiate these test cases :( Repository: rC Clang https://reviews.llvm.org/D51597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
usaxena95 updated this revision to Diff 163720.
usaxena95 added a comment.
- Moved helper function into anonymous namespace
Repository:
rC Clang
https://reviews.llvm.org/D51359
Files:
include/clang/Basic/VirtualFileSystem.h
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -16,7 +16,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/SourceMgr.h"
#include "gtest/gtest.h"
+#include "gmock/gmock.h"
#include
+#include
using namespace clang;
using namespace llvm;
@@ -697,6 +699,16 @@
NormalizedFS(/*UseNormalizedPaths=*/true) {}
};
+MATCHER_P2(IsHardLinkTo, FS, Target, "") {
+ Twine From = arg;
+ Twine To = Target;
+ auto OpenedFrom = FS->openFileForRead(From);
+ auto OpenedTo = FS->openFileForRead(To);
+ return !(OpenedFrom.getError() || OpenedTo.getError() ||
+ (*OpenedFrom)->status()->getUniqueID() !=
+ (*OpenedTo)->status()->getUniqueID());
+}
+
TEST_F(InMemoryFileSystemTest, IsEmpty) {
auto Stat = FS.status("/a");
ASSERT_EQ(Stat.getError(),errc::no_such_file_or_directory) << FS.toString();
@@ -958,6 +970,129 @@
ASSERT_EQ("../b/c", getPosixPath(It->getName()));
}
+TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) {
+ Twine FromLink = "/path/to/FROM/link";
+ Twine Target = "/path/to/TO/file";
+ StringRef content = "content of target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(FromLink, Target));
+ EXPECT_THAT(FromLink, IsHardLinkTo(, Target));
+ EXPECT_TRUE(FS.status(FromLink)->getSize() == FS.status(Target)->getSize());
+ EXPECT_TRUE(FS.getBufferForFile(FromLink)->get()->getBuffer() ==
+ FS.getBufferForFile(Target)->get()->getBuffer());
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkInChainPattern) {
+ StringRef content = "content of target file";
+ Twine link0 = "/path/to/0/link";
+ Twine link1 = "/path/to/1/link";
+ Twine link2 = "/path/to/2/link";
+ Twine target = "/path/to/target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(link2, target));
+ EXPECT_TRUE(FS.addHardLink(link1, link2));
+ EXPECT_TRUE(FS.addHardLink(link0, link1));
+ EXPECT_THAT(link0, IsHardLinkTo(, target));
+ EXPECT_THAT(link1, IsHardLinkTo(, target));
+ EXPECT_THAT(link2, IsHardLinkTo(, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToAFileThatWasNotAddedBefore) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromAFileThatWasAddedBefore) {
+ Twine link = "/path/to/link";
+ StringRef content_link = "content of link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ FS.addFile(link, 0, MemoryBuffer::getMemBuffer(content_link));
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddSameHardLinkMoreThanOnce) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithSameContent) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_TRUE(FS.addFile(link, 0, MemoryBuffer::getMemBuffer(content)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithDifferentContent) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ StringRef link_content = "different content of link";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_FALSE(FS.addFile(link, 0, MemoryBuffer::getMemBuffer(link_content)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToADirectory) {
+ Twine dir = "path/to/dummy/dir";
+ Twine link = "/path/to/link";
+ Twine dummy_file = dir + "/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(dummy_file, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_FALSE(FS.addHardLink(link, dir));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromADirectory) {
+ Twine dir = "path/to/dummy/dir";
+ Twine target = dir + "/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_FALSE(FS.addHardLink(dir,
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong created this revision.
martong added reviewers: a_sidorin, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: a.sidorin.
The init expression of a VarDecl is overwritten in the "To" context if we
import a VarDecl without an init expression (and with a definition). Please
refer to the added tests, especially InitAndDefinitionAreInDifferentTUs. This
patch fixes the malfunction by importing the whole Decl chain similarly as we
did that in case of FunctionDecls. We handle the init expression similarly to
a definition, alas only one init expression will be in the merged ast.
Repository:
rC Clang
https://reviews.llvm.org/D51597
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1828,13 +1828,62 @@
{
Decl *FromTU =
getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
-auto *FromD =
-FirstDeclMatcher().match(FromTU, functionDecl());
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
Import(FromD, Lang_CXX);
}
EXPECT_TRUE(Imported2->isUsed(false));
}
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag2) {
+ auto Pattern = varDecl(hasName("x"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl("int x = 1;", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+"int x = 1; int f() { return x; }", Lang_CXX, "input1.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag3) {
+ auto Pattern = varDecl(hasName("a"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+)", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+const int *f() { return ::a; } // requires storage,
+ // thus used flag will be set
+)", Lang_CXX, "input1.cc");
+auto *FromFunD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+auto *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ASSERT_TRUE(FromD->isUsed(false));
+Import(FromFunD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
auto Pattern = varDecl(hasName("x"));
@@ -3244,6 +3293,94 @@
EXPECT_TRUE(ToInitExpr->isGLValue());
}
+struct ImportVariables : ASTImporterTestBase {};
+
+TEST_P(ImportVariables, ImportOfOneDeclBringsInTheWholeChain) {
+ Decl *FromTU = getTuDecl(
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ const int A::a;
+ )", Lang_CXX, "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_NE(FromDWithInit, FromDWithDef);
+ ASSERT_EQ(FromDWithDef->getPreviousDecl() ,FromDWithInit);
+
+ auto *ToD0 = cast(Import(FromDWithInit, Lang_CXX11));
+ auto *ToD1 = cast(Import(FromDWithDef, Lang_CXX11));
+ ASSERT_TRUE(ToD0);
+ ASSERT_TRUE(ToD1);
+ EXPECT_NE(ToD0, ToD1);
+ EXPECT_EQ(ToD1->getPreviousDecl() ,ToD0);
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInDifferentTUs) {
+ auto StructA =
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ )";
+ Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+ Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a;", Lang_CXX,
+ "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl());
+ ASSERT_TRUE(FromDWithInit->getInit());
+ ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition());
+ ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+ ASSERT_FALSE(FromDWithDef->getInit());
+
+ auto *ToD = FirstDeclMatcher().match(
+ ToTU, varDecl(hasName("a"))); // Decl with init
+ ASSERT_TRUE(ToD->getInit());
+ ASSERT_FALSE(ToD->getDefinition());
+
+ auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
+ EXPECT_TRUE(ImportedD->getAnyInitializer());
+
[clang-tools-extra] r341319 - [clangd] Handle errors before checking for cancelltion
Author: ibiryukov
Date: Mon Sep 3 07:39:34 2018
New Revision: 341319
URL: http://llvm.org/viewvc/llvm-project?rev=341319=rev
Log:
[clangd] Handle errors before checking for cancelltion
To avoid hitting assertions in llvm::Expected destructor.
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=341319=341318=341319=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Mon Sep 3 07:39:34 2018
@@ -191,11 +191,10 @@ TaskHandle ClangdServer::codeComplete(Pa
auto Task = [PCHs, Pos, FS, CodeCompleteOpts,
this](Path File, Callback CB,
llvm::Expected IP) {
-if (isCancelled())
- return CB(llvm::make_error());
-
if (!IP)
return CB(IP.takeError());
+if (isCancelled())
+ return CB(llvm::make_error());
auto PreambleData = IP->Preamble;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341318: [clangd] Factor out the data-swapping functionality
from MemIndex/DexIndex. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51422?vs=163715=163716#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51422
Files:
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/MemIndex.h
clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestIndex.cpp
clang-tools-extra/trunk/unittests/clangd/TestIndex.h
Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
@@ -25,7 +25,6 @@
#include "Iterator.h"
#include "Token.h"
#include "Trigram.h"
-#include
namespace clang {
namespace clangd {
@@ -39,12 +38,24 @@
// index on disk and then load it if available.
class DexIndex : public SymbolIndex {
public:
- /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
- /// accessible as long as `Symbols` is kept alive.
- void build(std::shared_ptr> Syms);
-
- /// \brief Build index from a symbol slab.
- static std::unique_ptr build(SymbolSlab Slab);
+ // All symbols must outlive this index.
+ template DexIndex(Range &) {
+for (auto & : Symbols)
+ this->Symbols.push_back();
+buildIndex();
+ }
+ // Symbols are owned by BackingData, Index takes ownership.
+ template
+ DexIndex(Range &, Payload &)
+ : DexIndex(std::forward(Symbols)) {
+KeepAlive = std::shared_ptr(
+std::make_shared(std::move(BackingData)), nullptr);
+ }
+
+ /// Builds an index from a slab. The index takes ownership of the slab.
+ static std::unique_ptr build(SymbolSlab Slab) {
+return llvm::make_unique(Slab, std::move(Slab));
+ }
bool
fuzzyFind(const FuzzyFindRequest ,
@@ -60,19 +71,19 @@
size_t estimateMemoryUsage() const override;
private:
+ void buildIndex();
- mutable std::mutex Mutex;
-
- std::shared_ptr> Symbols /*GUARDED_BY(Mutex)*/;
- llvm::DenseMap LookupTable /*GUARDED_BY(Mutex)*/;
- llvm::DenseMap SymbolQuality /*GUARDED_BY(Mutex)*/;
+ std::vector Symbols;
+ llvm::DenseMap LookupTable;
+ llvm::DenseMap SymbolQuality;
// Inverted index is a mapping from the search token to the posting list,
// which contains all items which can be characterized by such search token.
// For example, if the search token is scope "std::", the corresponding
// posting list would contain all indices of symbols defined in namespace std.
// Inverted index is used to retrieve posting lists which are processed during
// the fuzzyFind process.
- llvm::DenseMap InvertedIndex /*GUARDED_BY(Mutex)*/;
+ llvm::DenseMap InvertedIndex;
+ std::shared_ptr KeepAlive; // poor man's move-only std::any
};
} // namespace dex
Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
@@ -36,48 +36,30 @@
} // namespace
-void DexIndex::build(std::shared_ptr> Syms) {
- llvm::DenseMap TempLookupTable;
- llvm::DenseMap TempSymbolQuality;
- for (const Symbol *Sym : *Syms) {
-TempLookupTable[Sym->ID] = Sym;
-TempSymbolQuality[Sym] = quality(*Sym);
+void DexIndex::buildIndex() {
+ for (const Symbol *Sym : Symbols) {
+LookupTable[Sym->ID] = Sym;
+SymbolQuality[Sym] = quality(*Sym);
}
// Symbols are sorted by symbol qualities so that items in the posting lists
// are stored in the descending order of symbol quality.
- std::sort(begin(*Syms), end(*Syms),
+ std::sort(begin(Symbols), end(Symbols),
[&](const Symbol *LHS, const Symbol *RHS) {
- return TempSymbolQuality[LHS] > TempSymbolQuality[RHS];
+ return SymbolQuality[LHS] > SymbolQuality[RHS];
});
- llvm::DenseMap TempInvertedIndex;
+
// Populate TempInvertedIndex with posting lists for index symbols.
- for (DocID SymbolRank = 0; SymbolRank < Syms->size(); ++SymbolRank) {
-const auto *Sym = (*Syms)[SymbolRank];
+ for (DocID SymbolRank = 0; SymbolRank < Symbols.size(); ++SymbolRank) {
+const auto *Sym = Symbols[SymbolRank];
for (const auto :
[clang-tools-extra] r341318 - [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
Author: sammccall
Date: Mon Sep 3 07:37:43 2018
New Revision: 341318
URL: http://llvm.org/viewvc/llvm-project?rev=341318=rev
Log:
[clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
Summary:
This is now handled by a wrapper class SwapIndex, so MemIndex/DexIndex can be
immutable and focus on their job.
Old and busted:
I have a MemIndex, which holds a shared_ptr>, which keeps the
symbol slab alive. I update by calling build(shared_ptr>).
New hotness: I have a SwapIndex, which holds a unique_ptr, which
holds a MemIndex, which holds a shared_ptr, which keeps backing
data alive.
I update by building a new MemIndex and calling SwapIndex::reset().
Reviewers: kbobyrev, ioeric
Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, mgrang, arphaman,
kadircet, cfe-commits
Differential Revision: https://reviews.llvm.org/D51422
Modified:
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/MemIndex.h
clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestIndex.cpp
clang-tools-extra/trunk/unittests/clangd/TestIndex.h
Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=341318=341317=341318=diff
==
--- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Mon Sep 3 07:37:43 2018
@@ -71,7 +71,9 @@ indexAST(ASTContext , std::shared_pt
}
FileIndex::FileIndex(std::vector URISchemes)
-: URISchemes(std::move(URISchemes)) {}
+: URISchemes(std::move(URISchemes)) {
+ reset(FSymbols.buildMemIndex());
+}
void FileSymbols::update(PathRef Path, std::unique_ptr Slab,
std::unique_ptr Occurrences) {
@@ -86,52 +88,32 @@ void FileSymbols::update(PathRef Path, s
FileToOccurrenceSlabs[Path] = std::move(Occurrences);
}
-std::shared_ptr> FileSymbols::allSymbols() {
- // The snapshot manages life time of symbol slabs and provides pointers of
all
- // symbols in all slabs.
- struct Snapshot {
-std::vector Pointers;
-std::vector> KeepAlive;
- };
- auto Snap = std::make_shared();
+std::unique_ptr FileSymbols::buildMemIndex() {
+ std::vector> Slabs;
+ std::vector> OccurrenceSlabs;
{
std::lock_guard Lock(Mutex);
-
-for (const auto : FileToSlabs) {
- Snap->KeepAlive.push_back(FileAndSlab.second);
- for (const auto : *FileAndSlab.second)
-Snap->Pointers.push_back();
-}
+for (const auto : FileToSlabs)
+ Slabs.push_back(FileAndSlab.second);
+for (const auto : FileToOccurrenceSlabs)
+ OccurrenceSlabs.push_back(FileAndOccurrenceSlab.second);
}
- auto *Pointers = >Pointers;
- // Use aliasing constructor to keep the snapshot alive along with the
- // pointers.
- return {std::move(Snap), Pointers};
-}
-
-std::shared_ptr FileSymbols::allOccurrences() const {
- // The snapshot manages life time of symbol occurrence slabs and provides
- // pointers to all occurrences in all occurrence slabs.
- struct Snapshot {
-MemIndex::OccurrenceMap Occurrences; // ID => {Occurrence}
-std::vector> KeepAlive;
- };
-
- auto Snap = std::make_shared();
- {
-std::lock_guard Lock(Mutex);
-
-for (const auto : FileToOccurrenceSlabs) {
- Snap->KeepAlive.push_back(FileAndSlab.second);
- for (const auto : *FileAndSlab.second) {
-auto = Snap->Occurrences[IDAndOccurrences.first];
-for (const auto : IDAndOccurrences.second)
- Occurrences.push_back();
- }
+ std::vector AllSymbols;
+ for (const auto : Slabs)
+for (const auto : *Slab)
+ AllSymbols.push_back();
+ MemIndex::OccurrenceMap AllOccurrences;
+ for (const auto : OccurrenceSlabs)
+for (const auto : *OccurrenceSlab) {
+ auto = AllOccurrences[Sym.first];
+ for (const auto : Sym.second)
+Entry.push_back();
}
- }
- return {std::move(Snap), >Occurrences};
+ // Index must keep the slabs alive.
+ return llvm::make_unique(
+ llvm::make_pointee_range(AllSymbols), std::move(AllOccurrences),
+ std::make_pair(std::move(Slabs), std::move(OccurrenceSlabs)));
}
void FileIndex::update(PathRef Path, ASTContext *AST,
@@ -148,30 +130,7 @@ void FileIndex::update(PathRef Path, AST
indexAST(*AST, PP, TopLevelDecls, URISchemes);
FSymbols.update(Path, std::move(Slab),
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
ioeric accepted this revision.
ioeric added inline comments.
Comment at: clangd/index/FileIndex.h:47
+ // The shared_ptr keeps the symbols alive.
+ std::shared_ptr buildMemIndex();
kbobyrev wrote:
> sammccall wrote:
> > ioeric wrote:
> > > Maybe avoid hardcoding the index name, so that we could potentially
> > > switch to use a different index implementation?
> > >
> > > We might also want to allow user to specify different index
> > > implementations for file index e.g. main file dynamic index might prefer
> > > MemIndex while Dex might be a better choice for the preamble index.
> > I don't think "returns an index of unspecified implementation" is the best
> > contract. MemIndex and DexIndex will both continue to exist, and they have
> > different performance characteristics (roughly fast build vs fast query).
> > So it should be up to the caller, no?
> We could make the index type a template parameter to allow users decide which
> one they want (also, this could be have a default value, e.g. `MemIndex` and
> later `DexIndex`). Would that be a viable solution?
Provide one interface for each index implementation sounds good to me. Thanks
for the clarification!
Comment at: clangd/index/Index.h:15
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Any.h"
#include "llvm/ADT/DenseMap.h"
nit: no longer needed?
Comment at: clangd/index/dex/DexIndex.h:42
+ // All symbols must outlive this index.
+ template DexIndex(Range &) {
+for (auto & : Symbols)
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > Why is this constructor needed? I think this could restrict the
> > > > flexibility of DexIndex initialization, in case we need to pass in
> > > > extra parameters here.
> > > I'm not sure exactly what you mean here.
> > >
> > > We need a way to specify the symbols to be indexed.
> > > Because these are now immutable, doing that in the constructor if
> > > possible is best.
> > >
> > > Previously this was a vector, but that sometimes required
> > > us to construct that big vector, dereference all those pointers, and
> > > throw away the vector. This signature is strictly more general (if you
> > > have a vector of pointers, you can pass `make_pointee_range > > Symbol>`)
> > >
> > > > in case we need to pass in extra parameters here.
> > > What stops us adding more parameters?
> > > What stops us adding more parameters?
> > I thought this template was added so that we could use it as a drop-in
> > replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I
> > might have overthought though.
> >
> > Thanks for the explanation!
> Sure, Dex and MemIndex had the same constructor signatures as each other
> before this patch, and they have the same as each other after this patch.
>
> That makes it convenient to use them interchangeably, but there's no hard
> requirement (no construction from templates etc).
Sounds good.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
sammccall added a comment. OK, I think this is good to go again. (A couple of seemingly-unrelated fixes to SymbolOccurrenceKind that I ran into in tests) Comment at: clangd/index/FileIndex.h:47 + // The shared_ptr keeps the symbols alive. + std::shared_ptr buildMemIndex(); ioeric wrote: > kbobyrev wrote: > > sammccall wrote: > > > ioeric wrote: > > > > Maybe avoid hardcoding the index name, so that we could potentially > > > > switch to use a different index implementation? > > > > > > > > We might also want to allow user to specify different index > > > > implementations for file index e.g. main file dynamic index might > > > > prefer MemIndex while Dex might be a better choice for the preamble > > > > index. > > > I don't think "returns an index of unspecified implementation" is the > > > best contract. MemIndex and DexIndex will both continue to exist, and > > > they have different performance characteristics (roughly fast build vs > > > fast query). So it should be up to the caller, no? > > We could make the index type a template parameter to allow users decide > > which one they want (also, this could be have a default value, e.g. > > `MemIndex` and later `DexIndex`). Would that be a viable solution? > Provide one interface for each index implementation sounds good to me. Thanks > for the clarification! I'm not really sure what problem templates solve there? If a caller wants a Dex index, we can just add a `buildDexIndex()` function. Templating and requiring these to always have the same constructor parameters seems fragile and unneccesary unless the dependency from FileIndex->Dex is really bad for some reason. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
sammccall updated this revision to Diff 163715.
sammccall added a comment.
Remove some more shared_ptr occurrences that aren't needed anymore
Update comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51422
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/dex/DexIndex.cpp
clangd/index/dex/DexIndex.h
unittests/clangd/DexIndexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestIndex.cpp
unittests/clangd/TestIndex.h
Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
// Creates Symbol instance and sets SymbolID to given QualifiedName.
Symbol symbol(llvm::StringRef QName);
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
- SymbolSlab Slab;
- std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
- std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
// Returns fully-qualified name out of given symbol.
std::string getQualifiedName(const Symbol );
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
return Sym;
}
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
SymbolSlab::Builder Slab;
for (llvm::StringRef QName : QualifiedNames)
Slab.insert(symbol(QName));
-
- auto Storage = std::make_shared();
- Storage->Slab = std::move(Slab).build();
- for (const auto : Storage->Slab)
-Storage->Pointers.push_back();
- if (WeakSymbols)
-*WeakSymbols = Storage;
- auto *Pointers = >Pointers;
- return {std::move(Storage), Pointers};
+ return std::move(Slab).build();
}
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
- std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
std::vector Names;
for (int i = Begin; i <= End; i++)
Names.push_back(std::to_string(i));
- return generateSymbols(Names, WeakSymbols);
+ return generateSymbols(Names);
}
std::string getQualifiedName(const Symbol ) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -17,18 +17,16 @@
#include "index/Merge.h"
#include "gtest/gtest.h"
+using testing::AllOf;
+using testing::ElementsAre;
using testing::Pointee;
using testing::UnorderedElementsAre;
-using testing::AllOf;
+using namespace llvm;
namespace clang {
namespace clangd {
namespace {
-std::shared_ptr emptyOccurrences() {
- return llvm::make_unique();
-}
-
MATCHER_P(Named, N, "") { return arg.Name == N; }
MATCHER_P(OccurrenceRange, Range, "") {
return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
@@ -54,155 +52,139 @@
EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym));
}
-TEST(MemIndexTest, MemIndexSymbolsRecycled) {
- MemIndex I;
- std::weak_ptr Symbols;
- I.build(generateNumSymbols(0, 10, ), emptyOccurrences());
- FuzzyFindRequest Req;
- Req.Query = "7";
- EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
+TEST(SwapIndexTest, OldIndexRecycled) {
+ auto Token = std::make_shared();
+ std::weak_ptr WeakToken = Token;
- EXPECT_FALSE(Symbols.expired());
- // Release old symbols.
- I.build(generateNumSymbols(0, 0), emptyOccurrences());
- EXPECT_TRUE(Symbols.expired());
+ SwapIndex S(make_unique(SymbolSlab(), MemIndex::OccurrenceMap(),
+std::move(Token)));
+ EXPECT_FALSE(WeakToken.expired()); // Current MemIndex keeps it alive.
+ S.reset(make_unique()); // Now the MemIndex is destroyed.
+ EXPECT_TRUE(WeakToken.expired()); // So the token is too.
}
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
sammccall updated this revision to Diff 163714.
sammccall added a comment.
Instead of returning shared_ptr, our implementations that have
backing data gain the ability to own that data (without coupling to its form).
Based on offline discussion with @ioeric.
Rebase to pick up SymbolOccurrence changes. The SymbolOccurrence parts of this
patch aren't terribly clean, but I think that's a problem for another day.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51422
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/dex/DexIndex.cpp
clangd/index/dex/DexIndex.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/DexIndexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestIndex.cpp
unittests/clangd/TestIndex.h
Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
// Creates Symbol instance and sets SymbolID to given QualifiedName.
Symbol symbol(llvm::StringRef QName);
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
- SymbolSlab Slab;
- std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
- std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
// Returns fully-qualified name out of given symbol.
std::string getQualifiedName(const Symbol );
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
return Sym;
}
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
SymbolSlab::Builder Slab;
for (llvm::StringRef QName : QualifiedNames)
Slab.insert(symbol(QName));
-
- auto Storage = std::make_shared();
- Storage->Slab = std::move(Slab).build();
- for (const auto : Storage->Slab)
-Storage->Pointers.push_back();
- if (WeakSymbols)
-*WeakSymbols = Storage;
- auto *Pointers = >Pointers;
- return {std::move(Storage), Pointers};
+ return std::move(Slab).build();
}
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
- std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
std::vector Names;
for (int i = Begin; i <= End; i++)
Names.push_back(std::to_string(i));
- return generateSymbols(Names, WeakSymbols);
+ return generateSymbols(Names);
}
std::string getQualifiedName(const Symbol ) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -17,18 +17,16 @@
#include "index/Merge.h"
#include "gtest/gtest.h"
+using testing::AllOf;
+using testing::ElementsAre;
using testing::Pointee;
using testing::UnorderedElementsAre;
-using testing::AllOf;
+using namespace llvm;
namespace clang {
namespace clangd {
namespace {
-std::shared_ptr emptyOccurrences() {
- return llvm::make_unique();
-}
-
MATCHER_P(Named, N, "") { return arg.Name == N; }
MATCHER_P(OccurrenceRange, Range, "") {
return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
@@ -54,155 +52,139 @@
EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym));
}
-TEST(MemIndexTest, MemIndexSymbolsRecycled) {
- MemIndex I;
- std::weak_ptr Symbols;
- I.build(generateNumSymbols(0, 10, ), emptyOccurrences());
- FuzzyFindRequest Req;
- Req.Query = "7";
- EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
+TEST(SwapIndexTest, OldIndexRecycled) {
+ auto Token = std::make_shared();
+ std::weak_ptr WeakToken = Token;
- EXPECT_FALSE(Symbols.expired());
- // Release old symbols.
- I.build(generateNumSymbols(0, 0), emptyOccurrences());
- EXPECT_TRUE(Symbols.expired());
+
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
usaxena95 updated this revision to Diff 163713.
usaxena95 marked 2 inline comments as done.
usaxena95 added a comment.
- applied suggested changes
Repository:
rC Clang
https://reviews.llvm.org/D51359
Files:
include/clang/Basic/VirtualFileSystem.h
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -16,7 +16,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/SourceMgr.h"
#include "gtest/gtest.h"
+#include "gmock/gmock.h"
#include
+#include
using namespace clang;
using namespace llvm;
@@ -697,6 +699,16 @@
NormalizedFS(/*UseNormalizedPaths=*/true) {}
};
+MATCHER_P2(IsHardLinkTo, FS, Target, "") {
+ Twine From = arg;
+ Twine To = Target;
+ auto OpenedFrom = FS->openFileForRead(From);
+ auto OpenedTo = FS->openFileForRead(To);
+ return !(OpenedFrom.getError() || OpenedTo.getError() ||
+ (*OpenedFrom)->status()->getUniqueID() !=
+ (*OpenedTo)->status()->getUniqueID());
+}
+
TEST_F(InMemoryFileSystemTest, IsEmpty) {
auto Stat = FS.status("/a");
ASSERT_EQ(Stat.getError(),errc::no_such_file_or_directory) << FS.toString();
@@ -958,6 +970,129 @@
ASSERT_EQ("../b/c", getPosixPath(It->getName()));
}
+TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) {
+ Twine FromLink = "/path/to/FROM/link";
+ Twine Target = "/path/to/TO/file";
+ StringRef content = "content of target";
+ FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(FromLink, Target));
+ EXPECT_THAT(FromLink, IsHardLinkTo(, Target));
+ EXPECT_TRUE(FS.status(FromLink)->getSize() == FS.status(Target)->getSize());
+ EXPECT_TRUE(FS.getBufferForFile(FromLink)->get()->getBuffer() ==
+ FS.getBufferForFile(Target)->get()->getBuffer());
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkInChainPattern) {
+ StringRef content = "content of target file";
+ Twine link0 = "/path/to/0/link";
+ Twine link1 = "/path/to/1/link";
+ Twine link2 = "/path/to/2/link";
+ Twine target = "/path/to/target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(link2, target));
+ EXPECT_TRUE(FS.addHardLink(link1, link2));
+ EXPECT_TRUE(FS.addHardLink(link0, link1));
+ EXPECT_THAT(link0, IsHardLinkTo(, target));
+ EXPECT_THAT(link1, IsHardLinkTo(, target));
+ EXPECT_THAT(link2, IsHardLinkTo(, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToAFileThatWasNotAddedBefore) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromAFileThatWasAddedBefore) {
+ Twine link = "/path/to/link";
+ StringRef content_link = "content of link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ FS.addFile(link, 0, MemoryBuffer::getMemBuffer(content_link));
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddSameHardLinkMoreThanOnce) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_FALSE(FS.addHardLink(link, target));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithSameContent) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_TRUE(FS.addFile(link, 0, MemoryBuffer::getMemBuffer(content)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddFileInPlaceOfAHardLinkWithDifferentContent) {
+ Twine link = "/path/to/link";
+ Twine target = "/path/to/target";
+ StringRef content = "content of target";
+ StringRef link_content = "different content of link";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_TRUE(FS.addHardLink(link, target));
+ EXPECT_FALSE(FS.addFile(link, 0, MemoryBuffer::getMemBuffer(link_content)));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkToADirectory) {
+ Twine dir = "path/to/dummy/dir";
+ Twine link = "/path/to/link";
+ Twine dummy_file = dir + "/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(dummy_file, 0, MemoryBuffer::getMemBuffer(content)));
+ EXPECT_FALSE(FS.addHardLink(link, dir));
+}
+
+TEST_F(InMemoryFileSystemTest, AddHardLinkFromADirectory) {
+ Twine dir = "path/to/dummy/dir";
+ Twine target = dir + "/target";
+ StringRef content = "content of target";
+ EXPECT_TRUE(FS.addFile(target, 0, MemoryBuffer::getMemBuffer(content)));
+
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
usaxena95 marked 16 inline comments as done. usaxena95 added a comment. Applied suggested changes. Comment at: lib/Basic/VirtualFileSystem.cpp:653 + // Cannot create HardLink from a directory. + if (HardLinkTarget && (!isa(HardLinkTarget) || Buffer)) +return false; ilya-biryukov wrote: > HardLinkTarget can only be `InMemoryFile`, so maybe change the type of the > parameter to `const InMemoryFile*`? > Clients will be responsible for making sure the target is actually a file, > but that seems fine, they already have to resolve the path to the target file. > Moved InMemoryFile from detail: namespace to detail namespace in order to be visible in VirtualFileSystem.h Comment at: unittests/Basic/VirtualFileSystemTest.cpp:705 + Twine To = Target; + auto OpenedFrom = FS->openFileForRead(From); + if (OpenedFrom.getError()) ilya-biryukov wrote: > This matcher is pretty complicated and tests multiple things. > If it fails, it might be hard to track down what exactly went wrong. > > Given the name, it seems reasonable for it to simply test that both paths > point to the same unique-id. > We don't have to test every other invariant (sizes are the same, buffers are > the same) on all the links we create, one test assertion per invariant should > be enough (sizes are the same, buffers are the same, etc) Removed the check for Buffer and size from the matcher. Added the test for size and buffer in the smaller AddHardLinkTest Repository: rC Clang https://reviews.llvm.org/D51359 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51533: [ASTImporter] Merge ExprBits
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rL341316: [ASTImporter] Merge ExprBits (authored by martong,
committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51533?vs=163494=163707#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51533
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -6817,9 +6817,9 @@
To->setSyntacticForm(ToSyntForm);
}
+ // Copy InitListExprBitfields, which are not handled in the ctor of
+ // InitListExpr.
To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
- To->setValueDependent(ILE->isValueDependent());
- To->setInstantiationDependent(ILE->isInstantiationDependent());
return To;
}
@@ -7164,6 +7164,19 @@
if (!ToS)
return nullptr;
+ if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+ }
+
// Record the imported declaration.
ImportedStmts[FromS] = ToS;
return ToS;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
unless(classTemplatePartialSpecializationDecl();
}
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+ Decl *TU = getTuDecl(
+ R"(
+ const int ();
+ void foo() { const int {init()}; }
+ )", Lang_CXX11, "input0.cc");
+ auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+ ASSERT_TRUE(FromD->getAnyInitializer());
+ auto *InitExpr = FromD->getAnyInitializer();
+ ASSERT_TRUE(InitExpr);
+ ASSERT_TRUE(InitExpr->isGLValue());
+
+ auto *ToD = Import(FromD, Lang_CXX11);
+ EXPECT_TRUE(ToD);
+ auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+ EXPECT_TRUE(ToInitExpr);
+ EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
struct DeclContextTest : ASTImporterTestBase {};
TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -6817,9 +6817,9 @@
To->setSyntacticForm(ToSyntForm);
}
+ // Copy InitListExprBitfields, which are not handled in the ctor of
+ // InitListExpr.
To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
- To->setValueDependent(ILE->isValueDependent());
- To->setInstantiationDependent(ILE->isInstantiationDependent());
return To;
}
@@ -7164,6 +7164,19 @@
if (!ToS)
return nullptr;
+ if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+ }
+
// Record the imported declaration.
ImportedStmts[FromS] = ToS;
return ToS;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
unless(classTemplatePartialSpecializationDecl();
}
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+ Decl *TU = getTuDecl(
+ R"(
+ const int ();
+ void foo() { const int {init()}; }
+ )", Lang_CXX11, "input0.cc");
+ auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+ ASSERT_TRUE(FromD->getAnyInitializer());
+ auto *InitExpr = FromD->getAnyInitializer();
+ ASSERT_TRUE(InitExpr);
+ ASSERT_TRUE(InitExpr->isGLValue());
+
+ auto *ToD = Import(FromD, Lang_CXX11);
+ EXPECT_TRUE(ToD);
+ auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+ EXPECT_TRUE(ToInitExpr);
+ EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
struct
[PATCH] D51533: [ASTImporter] Merge ExprBits
martong added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:3241 + auto *ToD = Import(FromD, Lang_CXX11); + ASSERT_TRUE(ToD); + auto *ToInitExpr = cast(ToD)->getAnyInitializer(); a_sidorin wrote: > EXPECT_TRUE (same below). Thanks, changed it. Repository: rL LLVM https://reviews.llvm.org/D51533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341316 - [ASTImporter] Merge ExprBits
Author: martong
Date: Mon Sep 3 06:10:53 2018
New Revision: 341316
URL: http://llvm.org/viewvc/llvm-project?rev=341316=rev
Log:
[ASTImporter] Merge ExprBits
Summary:
Some `Expr` classes set up default values for the `ExprBits` of `Stmt`. These
default values are then overwritten by the parser sometimes. One example is
`InitListExpr` which sets the value kind to be an rvalue in the ctor. However,
this bit may change after the `InitListExpr` is created. There may be other
expressions similar to `InitListExpr` in this sense, thus the safest solution
is to copy the expression bits.
The lack of copying `ExprBits` causes an assertion in the analyzer engine in a
specific case: Since the value kind is not imported, the analyzer engine
believes that the given InitListExpr is an rvalue, thus it creates a
nonloc::CompoundVal instead of creating memory region (as in case of an lvalue
reference).
Reviewers: a_sidorin, r.stahl, xazax.hun, a.sidorin
Subscribers: rnkovacs, dkrupp, cfe-commits
Differential Revision: https://reviews.llvm.org/D51533
Modified:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=341316=341315=341316=diff
==
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Sep 3 06:10:53 2018
@@ -6817,9 +6817,9 @@ Expr *ASTNodeImporter::VisitInitListExpr
To->setSyntacticForm(ToSyntForm);
}
+ // Copy InitListExprBitfields, which are not handled in the ctor of
+ // InitListExpr.
To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
- To->setValueDependent(ILE->isValueDependent());
- To->setInstantiationDependent(ILE->isInstantiationDependent());
return To;
}
@@ -7164,6 +7164,19 @@ Stmt *ASTImporter::Import(Stmt *FromS) {
if (!ToS)
return nullptr;
+ if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+ }
+
// Record the imported declaration.
ImportedStmts[FromS] = ToS;
return ToS;
Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=341316=341315=341316=diff
==
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Mon Sep 3 06:10:53 2018
@@ -3225,6 +3225,25 @@ TEST_P(ASTImporterTestBase, ClassTemplat
unless(classTemplatePartialSpecializationDecl();
}
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+ Decl *TU = getTuDecl(
+ R"(
+ const int ();
+ void foo() { const int {init()}; }
+ )", Lang_CXX11, "input0.cc");
+ auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+ ASSERT_TRUE(FromD->getAnyInitializer());
+ auto *InitExpr = FromD->getAnyInitializer();
+ ASSERT_TRUE(InitExpr);
+ ASSERT_TRUE(InitExpr->isGLValue());
+
+ auto *ToD = Import(FromD, Lang_CXX11);
+ EXPECT_TRUE(ToD);
+ auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+ EXPECT_TRUE(ToInitExpr);
+ EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
struct DeclContextTest : ASTImporterTestBase {};
TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341314 - [Index] Update tests allowing double4 type to be "invalid"
Author: AlexeySotkin
Date: Mon Sep 3 05:43:26 2018
New Revision: 341314
URL: http://llvm.org/viewvc/llvm-project?rev=341314=rev
Log:
[Index] Update tests allowing double4 type to be "invalid"
Fixes test failure after r341309
Modified:
cfe/trunk/test/Index/opencl-types.cl
Modified: cfe/trunk/test/Index/opencl-types.cl
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/opencl-types.cl?rev=341314=341313=341314=diff
==
--- cfe/trunk/test/Index/opencl-types.cl (original)
+++ cfe/trunk/test/Index/opencl-types.cl Mon Sep 3 05:43:26 2018
@@ -21,7 +21,7 @@ void kernel testFloatTypes() {
// CHECK: VarDecl=scalarFloat:13:9 (Definition) [type=float] [typekind=Float]
[isPOD=1]
// CHECK: VarDecl=vectorFloat:14:10 (Definition) [type=float4]
[typekind=Typedef] [canonicaltype=float __attribute__((ext_vector_type(4)))]
[canonicaltypekind=Unexposed] [isPOD=1]
// CHECK: VarDecl=scalarDouble:15:10 (Definition){{( \(invalid\))?}}
[type=double] [typekind=Double] [isPOD=1]
-// CHECK: VarDecl=vectorDouble:16:11 (Definition) [type=double4]
[typekind=Typedef] [canonicaltype=double __attribute__((ext_vector_type(4)))]
[canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorDouble:16:11 (Definition){{( \(invalid\))?}}
[type=double4] [typekind=Typedef] [canonicaltype=double
__attribute__((ext_vector_type(4)))] [canonicaltypekind=Unexposed] [isPOD=1]
#pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : enable
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341309 - [OpenCL] Traverse vector types for ocl extensions support
Author: AlexeySotkin
Date: Mon Sep 3 04:43:22 2018
New Revision: 341309
URL: http://llvm.org/viewvc/llvm-project?rev=341309=rev
Log:
[OpenCL] Traverse vector types for ocl extensions support
Summary:
Given the following kernel:
__kernel void foo() {
double d;
double4 dd;
}
and cl_khr_fp64 is disabled, the compilation would fail due to
the presence of 'double d', but when removed, it passes.
The expectation is that extended vector types of unsupported types
will also be unsupported.
The patch adds the check for this scenario.
Patch by: Ofir Cohen
Reviewers: bader, Anastasia, AlexeySotkin, yaxunl
Reviewed By: Anastasia
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D51296
Modified:
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/test/SemaOpenCL/extensions.cl
Modified: cfe/trunk/lib/Sema/Sema.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=341309=341308=341309=diff
==
--- cfe/trunk/lib/Sema/Sema.cpp (original)
+++ cfe/trunk/lib/Sema/Sema.cpp Mon Sep 3 04:43:22 2018
@@ -1889,6 +1889,14 @@ bool Sema::checkOpenCLDisabledTypeDeclSp
if (auto TagT = dyn_cast(QT.getCanonicalType().getTypePtr()))
Decl = TagT->getDecl();
auto Loc = DS.getTypeSpecTypeLoc();
+
+ // Check extensions for vector types.
+ // e.g. double4 is not allowed when cl_khr_fp64 is absent.
+ if (QT->isExtVectorType()) {
+auto TypePtr = QT->castAs()->getElementType().getTypePtr();
+return checkOpenCLDisabledTypeOrDecl(TypePtr, Loc, QT, OpenCLTypeExtMap);
+ }
+
if (checkOpenCLDisabledTypeOrDecl(Decl, Loc, QT, OpenCLDeclExtMap))
return true;
Modified: cfe/trunk/test/SemaOpenCL/extensions.cl
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaOpenCL/extensions.cl?rev=341309=341308=341309=diff
==
--- cfe/trunk/test/SemaOpenCL/extensions.cl (original)
+++ cfe/trunk/test/SemaOpenCL/extensions.cl Mon Sep 3 04:43:22 2018
@@ -70,6 +70,13 @@ void f2(void) {
// expected-error@-2{{use of type 'double' requires cl_khr_fp64 extension to
be enabled}}
#endif
+ typedef double double4 __attribute__((ext_vector_type(4)));
+ double4 d4 = {0.0f, 2.0f, 3.0f, 1.0f};
+#ifdef NOFP64
+// expected-error@-3 {{use of type 'double' requires cl_khr_fp64 extension to
be enabled}}
+// expected-error@-3 {{use of type 'double4' (vector of 4 'double' values)
requires cl_khr_fp64 extension to be enabled}}
+#endif
+
(void) 1.0;
#ifdef NOFP64
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r341312 - [Aarch64] Fix linker emulation for Aarch64 big endian
Author: psmith
Date: Mon Sep 3 05:36:32 2018
New Revision: 341312
URL: http://llvm.org/viewvc/llvm-project?rev=341312=rev
Log:
[Aarch64] Fix linker emulation for Aarch64 big endian
This patch fixes target linker emulation for aarch64 big endian.
aarch64_be_linux is not recognized by gnu ld. The equivalent emulation
mode supported by gnu ld is aarch64linuxb.
Patch by: Bharathi Seshadri
Reviewed by: Peter Smith
Differential Revision: https://reviews.llvm.org/D42930
Modified:
cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
cfe/trunk/test/Driver/linux-ld.c
Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=341312=341311=341312=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Mon Sep 3 05:36:32 2018
@@ -237,7 +237,7 @@ static const char *getLDMOption(const ll
case llvm::Triple::aarch64:
return "aarch64linux";
case llvm::Triple::aarch64_be:
-return "aarch64_be_linux";
+return "aarch64linuxb";
case llvm::Triple::arm:
case llvm::Triple::thumb:
return "armelf_linux_eabi";
Modified: cfe/trunk/test/Driver/linux-ld.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/linux-ld.c?rev=341312=341311=341312=diff
==
--- cfe/trunk/test/Driver/linux-ld.c (original)
+++ cfe/trunk/test/Driver/linux-ld.c Mon Sep 3 05:36:32 2018
@@ -1754,6 +1754,14 @@
// CHECK-ARMV7EB: "--be8"
// CHECK-ARMV7EB: "-m" "armelfb_linux_eabi"
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64_be-unknown-linux \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: | FileCheck --check-prefix=CHECK-AARCH64BE %s
+// CHECK-AARCH64BE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-AARCH64BE: "-m" "aarch64linuxb"
+
// Check dynamic-linker for musl-libc
// RUN: %clang %s -### -o %t.o 2>&1 \
// RUN: --target=i386-pc-linux-musl \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51481: [clangd] Implement proximity path boosting for Dex
ioeric added inline comments.
Comment at: clang-tools-extra/;:1
+//===--- Token.cpp - Symbol Search primitive *- C++
-*-===//
+// The LLVM Compiler Infrastructure
Unintended change?
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:137
+ BoostingIterators.push_back(
+ createBoost(create(It->second), PARENTS_TO_BOOST + 1 -
P.second));
+ }
ioeric wrote:
> `PARENTS_TO_BOOST + 1 - P.second` seems to be too much boosting. We should
> align the boosting function with the one we use in clangd/Quality.cpp, e.g.
> proximity score should be would be in the range [0, 1], and we can boost by
> `1+proximity`.
The `FIXME` is now outdated?
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:143
+ float BoostFactor =
+ std::exp(Distance * 0.4f / FileDistanceOptions().UpCost);
+ BoostingIterators.push_back(
`x` in `std::exp(x)` should be negated so that the range is (0, 1]. And
`BoostFactor` should be `1+(0,1]`.
I'm not sure if dividing by UpCost is correct here (and similarly in
Quality.cpp), because you would want lower score for larger UpCost?
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:36
+ size_t Limit = 5;
+ while (!Path.empty() && Limit--) {
+// FIXME(kbobyrev): Parsing and encoding path to URIs is not necessary and
For the original URI, we could simply add it to the result outside of the loop
(and `--Limit`) to save one iteration?
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:54
+// should be just skipped.
+if (!static_cast(URI)) {
+ // Ignore the error.
nit: just `if (!URI)`
Comment at: clang-tools-extra/clangd/index/dex/Token.h:64
+/// Example: "file:///path/to/clang-tools-extra/clangd/index/SymbolIndex.h"
+/// and all its parents.
+PathURI,
nit: not necessarily *all* parents right?
Comment at: clang-tools-extra/clangd/index/dex/Token.h:65
+/// and all its parents.
+PathURI,
/// Internal Token type for invalid/special tokens, e.g. empty tokens for
Maybe just `URI`? Or `ProximityURI`?
Comment at: clang-tools-extra/clangd/index/dex/Token.h:96
+/// Should be used within the index build process.
+std::vector generateProximityPathURIs(llvm::StringRef Path);
+
nit: s/Path/URIStr/ for the argument.
nit: Just `generateProximityURIs`? Same below.
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:443
+TEST(DexSearchTokens, QueryProximityDistances) {
+ EXPECT_THAT(generateQueryProximityPathURIs(testRoot() + "/a/b/c/d/e/f/g.h",
+ URISchemes),
This doesn't seem to work on windows?
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:620
+TEST(DexIndexTest, ProximityPathsBoosting) {
+ DexIndex I(URISchemes);
It's unclear what this is testing. Intuitively, you would want a smoke test
that checks a symbol with better proximity is ranked higher (when all other
signals are the same).
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:653
+ // FuzzyFind request comes from the file which is far from the root.
+ Req.ProximityPaths.push_back(testRoot() + "/a/b/c/d/e/f/file.h");
+
again, watch out for windows when hard-coding paths :)
Comment at: clang-tools-extra/unittests/clangd/TestFS.cpp:67
-const char *testRoot() {
+std::string testRoot() {
#ifdef _WIN32
Why this change?
https://reviews.llvm.org/D51481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.
kbobyrev accepted this revision. kbobyrev added a comment. This revision is now accepted and ready to land. Other than a hard-coded `buildMemIndex()` in `FileIndex`, I don't have any concerns. That's just a tiny piece, if @ioeric doesn't have any concerns about that too, I think it's good to land. The code should look cleaner in multiple places now, thanks for the patch! Comment at: clangd/index/FileIndex.h:47 + // The shared_ptr keeps the symbols alive. + std::shared_ptr buildMemIndex(); sammccall wrote: > ioeric wrote: > > Maybe avoid hardcoding the index name, so that we could potentially switch > > to use a different index implementation? > > > > We might also want to allow user to specify different index implementations > > for file index e.g. main file dynamic index might prefer MemIndex while Dex > > might be a better choice for the preamble index. > I don't think "returns an index of unspecified implementation" is the best > contract. MemIndex and DexIndex will both continue to exist, and they have > different performance characteristics (roughly fast build vs fast query). So > it should be up to the caller, no? We could make the index type a template parameter to allow users decide which one they want (also, this could be have a default value, e.g. `MemIndex` and later `DexIndex`). Would that be a viable solution? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51296: [OpenCL] Traverse vector types for ocl extensions support
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341309: [OpenCL] Traverse vector types for ocl extensions
support (authored by AlexeySotkin, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51296?vs=163287=163691#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51296
Files:
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/test/SemaOpenCL/extensions.cl
Index: cfe/trunk/test/SemaOpenCL/extensions.cl
===
--- cfe/trunk/test/SemaOpenCL/extensions.cl
+++ cfe/trunk/test/SemaOpenCL/extensions.cl
@@ -70,6 +70,13 @@
// expected-error@-2{{use of type 'double' requires cl_khr_fp64 extension to
be enabled}}
#endif
+ typedef double double4 __attribute__((ext_vector_type(4)));
+ double4 d4 = {0.0f, 2.0f, 3.0f, 1.0f};
+#ifdef NOFP64
+// expected-error@-3 {{use of type 'double' requires cl_khr_fp64 extension to
be enabled}}
+// expected-error@-3 {{use of type 'double4' (vector of 4 'double' values)
requires cl_khr_fp64 extension to be enabled}}
+#endif
+
(void) 1.0;
#ifdef NOFP64
Index: cfe/trunk/lib/Sema/Sema.cpp
===
--- cfe/trunk/lib/Sema/Sema.cpp
+++ cfe/trunk/lib/Sema/Sema.cpp
@@ -1889,6 +1889,14 @@
if (auto TagT = dyn_cast(QT.getCanonicalType().getTypePtr()))
Decl = TagT->getDecl();
auto Loc = DS.getTypeSpecTypeLoc();
+
+ // Check extensions for vector types.
+ // e.g. double4 is not allowed when cl_khr_fp64 is absent.
+ if (QT->isExtVectorType()) {
+auto TypePtr = QT->castAs()->getElementType().getTypePtr();
+return checkOpenCLDisabledTypeOrDecl(TypePtr, Loc, QT, OpenCLTypeExtMap);
+ }
+
if (checkOpenCLDisabledTypeOrDecl(Decl, Loc, QT, OpenCLDeclExtMap))
return true;
Index: cfe/trunk/test/SemaOpenCL/extensions.cl
===
--- cfe/trunk/test/SemaOpenCL/extensions.cl
+++ cfe/trunk/test/SemaOpenCL/extensions.cl
@@ -70,6 +70,13 @@
// expected-error@-2{{use of type 'double' requires cl_khr_fp64 extension to be enabled}}
#endif
+ typedef double double4 __attribute__((ext_vector_type(4)));
+ double4 d4 = {0.0f, 2.0f, 3.0f, 1.0f};
+#ifdef NOFP64
+// expected-error@-3 {{use of type 'double' requires cl_khr_fp64 extension to be enabled}}
+// expected-error@-3 {{use of type 'double4' (vector of 4 'double' values) requires cl_khr_fp64 extension to be enabled}}
+#endif
+
(void) 1.0;
#ifdef NOFP64
Index: cfe/trunk/lib/Sema/Sema.cpp
===
--- cfe/trunk/lib/Sema/Sema.cpp
+++ cfe/trunk/lib/Sema/Sema.cpp
@@ -1889,6 +1889,14 @@
if (auto TagT = dyn_cast(QT.getCanonicalType().getTypePtr()))
Decl = TagT->getDecl();
auto Loc = DS.getTypeSpecTypeLoc();
+
+ // Check extensions for vector types.
+ // e.g. double4 is not allowed when cl_khr_fp64 is absent.
+ if (QT->isExtVectorType()) {
+auto TypePtr = QT->castAs()->getElementType().getTypePtr();
+return checkOpenCLDisabledTypeOrDecl(TypePtr, Loc, QT, OpenCLTypeExtMap);
+ }
+
if (checkOpenCLDisabledTypeOrDecl(Decl, Loc, QT, OpenCLDeclExtMap))
return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall added a comment. This still needs to be synced to head to account for Symbol changes (multi-includes) but those changes are pretty obvious/mechanical. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall updated this revision to Diff 163688.
sammccall added a comment.
Revert unused changes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51585
Files:
clangd/CMakeLists.txt
clangd/RIFF.cpp
clangd/RIFF.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/RIFFTests.cpp
unittests/clangd/SerializationTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -45,7 +45,6 @@
MATCHER_P(Labeled, Label, "") {
return (arg.Name + arg.Signature).str() == Label;
}
-MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
MATCHER_P(Snippet, S, "") {
@@ -740,75 +739,6 @@
Snippet("ff(${1:int x}, ${2:double y})";
}
-TEST_F(SymbolCollectorTest, YAMLConversions) {
- const std::string YAML1 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
-Name: 'Foo1'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/foo.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:true
-Documentation:'Foo doc'
-ReturnType:'int'
-...
-)";
- const std::string YAML2 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
-Name: 'Foo2'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/bar.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:false
-Signature:'-sig'
-CompletionSnippetSuffix:'-snippet'
-...
-)";
-
- auto Symbols1 = symbolsFromYAML(YAML1);
-
- EXPECT_THAT(Symbols1,
- UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
- Doc("Foo doc"), ReturnType("int"),
- DeclURI("file:///path/foo.h"),
- ForCodeCompletion(true;
- auto Symbols2 = symbolsFromYAML(YAML2);
- EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
-QName("clang::Foo2"), Labeled("Foo2-sig"),
-Not(HasReturnType()), DeclURI("file:///path/bar.h"),
-ForCodeCompletion(false;
-
- std::string ConcatenatedYAML;
- {
-llvm::raw_string_ostream OS(ConcatenatedYAML);
-SymbolsToYAML(Symbols1, OS);
-SymbolsToYAML(Symbols2, OS);
- }
- auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
- EXPECT_THAT(ConcatenatedSymbols,
- UnorderedElementsAre(QName("clang::Foo1"),
- QName("clang::Foo2")));
-}
-
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
Index: unittests/clangd/SerializationTests.cpp
===
--- /dev/null
+++ unittests/clangd/SerializationTests.cpp
@@ -0,0 +1,127 @@
+//===-- SerializationTests.cpp - Binary and YAML serialization unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/Serialization.h"
+#include "index/SymbolYAML.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
+namespace clang {
+namespace clangd {
+namespace {
+
+const char *YAML1 = R"(
+---
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
+Name: 'Foo1'
+Scope: 'clang::'
+SymInfo:
+ Kind:Function
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI:file:///path/foo.h
+ Start:
+Line: 1
+Column: 0
+ End:
+Line: 1
+Column: 1
+IsIndexedForCodeCompletion:true
+Documentation:'Foo doc'
+ReturnType:'int'
+...
+)";
+
+const char *YAML2 = R"(
+---
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
+Name: 'Foo2'
+Scope: 'clang::'
+SymInfo:
+ Kind:Function
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI:file:///path/bar.h
+ Start:
+Line: 1
+Column: 0
+ End:
+Line: 1
+Column: 1
+IsIndexedForCodeCompletion:false
+Signature:'-sig'
+CompletionSnippetSuffix:'-snippet'
+...
[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous,
MaskRay, ilya-biryukov, mgorny.
This is intended to replace the current YAML format for general use.
It's ~10x more compact than YAML, and ~40% more compact than gzipped YAML:
llvmidx.riff = 20M, llvmidx.yaml = 272M, llvmidx.yaml.gz = 32M
It's also simpler/faster to read and write.
The format is a RIFF container (chunks of (type, size, data)) with:
- a compressed string table
- simple binary encoding of symbols (with varints for compactness)
It can be extended to include occurrences, Dex posting lists, etc.
There's no rich backwards-compatibility scheme, but a version number is included
so we can detect incompatible files and do ad-hoc back-compat.
Alternatives considered:
- compressed YAML or JSON: bulky and slow to load
- llvm bitstream: confusing model and libraries are hard to use. My attempt
produced slightly larger files, and the code was longer and slower.
- protobuf or similar: would be really nice (esp for back-compat) but the
dependency is a big hassle
- ad-hoc binary format without a container: it seems clear we're going to add
posting lists and occurrences here, and that they will benefit from sharing a
string table. The container makes it easy to debug these pieces in isolation,
and make them optional.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51585
Files:
clangd/CMakeLists.txt
clangd/RIFF.cpp
clangd/RIFF.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/RIFFTests.cpp
unittests/clangd/SerializationTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -45,7 +45,6 @@
MATCHER_P(Labeled, Label, "") {
return (arg.Name + arg.Signature).str() == Label;
}
-MATCHER(HasReturnType, "") { return !arg.ReturnType.empty(); }
MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
MATCHER_P(Snippet, S, "") {
@@ -740,75 +739,6 @@
Snippet("ff(${1:int x}, ${2:double y})";
}
-TEST_F(SymbolCollectorTest, YAMLConversions) {
- const std::string YAML1 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
-Name: 'Foo1'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/foo.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:true
-Documentation:'Foo doc'
-ReturnType:'int'
-...
-)";
- const std::string YAML2 = R"(
-ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
-Name: 'Foo2'
-Scope: 'clang::'
-SymInfo:
- Kind:Function
- Lang:Cpp
-CanonicalDeclaration:
- FileURI:file:///path/bar.h
- Start:
-Line: 1
-Column: 0
- End:
-Line: 1
-Column: 1
-IsIndexedForCodeCompletion:false
-Signature:'-sig'
-CompletionSnippetSuffix:'-snippet'
-...
-)";
-
- auto Symbols1 = symbolsFromYAML(YAML1);
-
- EXPECT_THAT(Symbols1,
- UnorderedElementsAre(AllOf(QName("clang::Foo1"), Labeled("Foo1"),
- Doc("Foo doc"), ReturnType("int"),
- DeclURI("file:///path/foo.h"),
- ForCodeCompletion(true;
- auto Symbols2 = symbolsFromYAML(YAML2);
- EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
-QName("clang::Foo2"), Labeled("Foo2-sig"),
-Not(HasReturnType()), DeclURI("file:///path/bar.h"),
-ForCodeCompletion(false;
-
- std::string ConcatenatedYAML;
- {
-llvm::raw_string_ostream OS(ConcatenatedYAML);
-SymbolsToYAML(Symbols1, OS);
-SymbolsToYAML(Symbols2, OS);
- }
- auto ConcatenatedSymbols = symbolsFromYAML(ConcatenatedYAML);
- EXPECT_THAT(ConcatenatedSymbols,
- UnorderedElementsAre(QName("clang::Foo1"),
- QName("clang::Foo2")));
-}
-
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
Index: unittests/clangd/SerializationTests.cpp
===
--- /dev/null
+++ unittests/clangd/SerializationTests.cpp
@@ -0,0 +1,127 @@
+//===-- SerializationTests.cpp - Binary and YAML serialization unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
devnexen added a comment. ping https://reviews.llvm.org/D49722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51481: [clangd] Implement proximity path boosting for Dex
kbobyrev updated this revision to Diff 163680.
kbobyrev marked 17 inline comments as done.
kbobyrev added a comment.
Address a round of comments, refactor code.
https://reviews.llvm.org/D51481
Files:
clang-tools-extra/;
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/index/dex/DexIndex.cpp
clang-tools-extra/clangd/index/dex/DexIndex.h
clang-tools-extra/clangd/index/dex/Token.cpp
clang-tools-extra/clangd/index/dex/Token.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/unittests/clangd/DexIndexTests.cpp
clang-tools-extra/unittests/clangd/TestFS.cpp
clang-tools-extra/unittests/clangd/TestFS.h
Index: clang-tools-extra/unittests/clangd/TestFS.h
===
--- clang-tools-extra/unittests/clangd/TestFS.h
+++ clang-tools-extra/unittests/clangd/TestFS.h
@@ -59,7 +59,7 @@
};
// Returns an absolute (fake) test directory for this OS.
-const char *testRoot();
+std::string testRoot();
// Returns a suitable absolute path for this OS.
std::string testPath(PathRef File);
Index: clang-tools-extra/unittests/clangd/TestFS.cpp
===
--- clang-tools-extra/unittests/clangd/TestFS.cpp
+++ clang-tools-extra/unittests/clangd/TestFS.cpp
@@ -64,7 +64,7 @@
FileName, std::move(CommandLine), "")};
}
-const char *testRoot() {
+std::string testRoot() {
#ifdef _WIN32
return "C:\\clangd-test";
#else
Index: clang-tools-extra/unittests/clangd/DexIndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexIndexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexIndexTests.cpp
@@ -7,6 +7,8 @@
//
//===--===//
+#include "FuzzyMatch.h"
+#include "TestFS.h"
#include "TestIndex.h"
#include "index/Index.h"
#include "index/Merge.h"
@@ -29,6 +31,8 @@
namespace dex {
namespace {
+std::vector URISchemes = {"unittest"};
+
std::vector consumeIDs(Iterator ) {
auto IDAndScore = consume(It);
std::vector IDs(IDAndScore.size());
@@ -325,14 +329,33 @@
}
testing::Matcher>
-trigramsAre(std::initializer_list Trigrams) {
+tokensAre(std::initializer_list Strings, Token::Kind Kind) {
std::vector Tokens;
- for (const auto : Trigrams) {
-Tokens.push_back(Token(Token::Kind::Trigram, Symbols));
+ for (const auto : Strings) {
+Tokens.push_back(Token(Kind, TokenData));
}
return testing::UnorderedElementsAreArray(Tokens);
}
+testing::Matcher>
+trigramsAre(std::initializer_list Trigrams) {
+ return tokensAre(Trigrams, Token::Kind::Trigram);
+}
+
+testing::Matcher>
+pathsAre(std::initializer_list Paths) {
+ return tokensAre(Paths, Token::Kind::PathURI);
+}
+
+testing::Matcher>
+proximityPathsAre(std::initializer_list ProximityPaths) {
+ std::vector Result;
+ for (const auto : ProximityPaths) {
+Result.emplace_back(Token::Kind::PathURI, Path);
+ }
+ return testing::UnorderedElementsAreArray(Result);
+}
+
TEST(DexIndexTrigrams, IdentifierTrigrams) {
EXPECT_THAT(generateIdentifierTrigrams("X86"),
trigramsAre({"x86", "x$$", "x8$"}));
@@ -407,8 +430,27 @@
"hij", "ijk", "jkl", "klm"}));
}
+TEST(DexSearchTokens, SymbolPath) {
+ EXPECT_THAT(generateProximityPathURIs(
+ "unittest:///clang-tools-extra/clangd/index/Token.h"),
+ pathsAre({"unittest:///clang-tools-extra/clangd/index/Token.h",
+"unittest:///clang-tools-extra/clangd/index",
+"unittest:///clang-tools-extra/clangd",
+"unittest:///clang-tools-extra", "unittest:///"}));
+}
+
+TEST(DexSearchTokens, QueryProximityDistances) {
+ EXPECT_THAT(generateQueryProximityPathURIs(testRoot() + "/a/b/c/d/e/f/g.h",
+ URISchemes),
+ proximityPathsAre({{"unittest:///a/b/c/d/e/f/g.h"},
+ {"unittest:///a/b/c/d/e/f"},
+ {"unittest:///a/b/c/d/e"},
+ {"unittest:///a/b/c/d"},
+ {"unittest:///a/b/c"}}));
+}
+
TEST(DexIndex, Lookup) {
- DexIndex I;
+ DexIndex I(URISchemes);
I.build(generateSymbols({"ns::abc", "ns::xyz"}));
EXPECT_THAT(lookup(I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc"));
EXPECT_THAT(lookup(I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}),
@@ -419,7 +461,7 @@
}
TEST(DexIndex, FuzzyFind) {
- DexIndex Index;
+ DexIndex Index(URISchemes);
Index.build(generateSymbols({"ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC",
"other::ABC", "other::A"}));
FuzzyFindRequest Req;
@@ -442,7 +484,7 @@
}
TEST(DexIndexTest, FuzzyMatchQ) {
- DexIndex I;
+ DexIndex I(URISchemes);
I.build(
generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}));
[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341304: [clangd] Support multiple #include headers in one
symbol. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D51291
Files:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/Merge.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -54,7 +54,13 @@
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
-MATCHER_P(IncludeHeader, P, "") { return arg.IncludeHeader == P; }
+MATCHER_P(IncludeHeader, P, "") {
+ return (arg.IncludeHeaders.size() == 1) &&
+ (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
MATCHER_P(DeclRange, Pos, "") {
return std::tie(arg.CanonicalDeclaration.Start.Line,
arg.CanonicalDeclaration.Start.Column,
@@ -760,6 +766,11 @@
IsIndexedForCodeCompletion:true
Documentation:'Foo doc'
ReturnType:'int'
+IncludeHeaders:
+ - Header:'include1'
+References:7
+ - Header:'include2'
+References:3
...
)";
const std::string YAML2 = R"(
@@ -791,6 +802,10 @@
Doc("Foo doc"), ReturnType("int"),
DeclURI("file:///path/foo.h"),
ForCodeCompletion(true;
+ auto = *Symbols1.begin();
+ EXPECT_THAT(Sym1.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
+ IncludeHeaderWithRef("include2", 3u)));
auto Symbols2 = symbolsFromYAML(YAML2);
EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
QName("clang::Foo2"), Labeled("Foo2-sig"),
@@ -812,9 +827,10 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
- IncludeHeader(TestHeaderURI;
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI;
+ EXPECT_THAT(Symbols.begin()->IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
}
#ifndef _WIN32
Index: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
@@ -223,7 +223,7 @@
Req.Query = "";
bool SeenSymbol = false;
M.fuzzyFind(Req, [&](const Symbol ) {
-EXPECT_TRUE(Sym.IncludeHeader.empty());
+EXPECT_TRUE(Sym.IncludeHeaders.empty());
SeenSymbol = true;
});
EXPECT_TRUE(SeenSymbol);
Index: clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
@@ -306,6 +306,46 @@
FileURI("unittest:///test2.cc";
}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
+
+TEST(MergeTest, MergeIncludesOnDifferentDefinitions) {
+ Symbol L, R;
+ L.Name = "left";
+ R.Name = "right";
+ L.ID = R.ID = SymbolID("hello");
+ L.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("new", 1);
+
+ // Both have no definition.
+ Symbol M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+
[clang-tools-extra] r341304 - [clangd] Support multiple #include headers in one symbol.
Author: ioeric
Date: Mon Sep 3 03:18:21 2018
New Revision: 341304
URL: http://llvm.org/viewvc/llvm-project?rev=341304=rev
Log:
[clangd] Support multiple #include headers in one symbol.
Summary:
Currently, a symbol can have only one #include header attached, which
might not work well if the symbol can be imported via different #includes
depending
on where it's used. This patch stores multiple #include headers (with #
references)
for each symbol, so that CodeCompletion can decide which include to insert.
In this patch, code completion simply picks the most popular include as the
default inserted header. We also return all possible includes and their edits
in the `CodeCompletion` results.
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: mgrang, ilya-biryukov, MaskRay, jkorous, arphaman, kadircet,
cfe-commits
Differential Revision: https://reviews.llvm.org/D51291
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/Merge.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=341304=341303=341304=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Mon Sep 3 03:18:21 2018
@@ -44,10 +44,13 @@
#include "clang/Sema/Sema.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/ScopedPrinter.h"
+#include
+#include
#include
// We log detailed candidate here if you run with -debug-only=codecomplete.
@@ -247,6 +250,7 @@ struct CompletionCandidate {
// We may have a result from Sema, from the index, or both.
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ llvm::SmallVector RankedIncludeHeaders;
// States whether this item is an override suggestion.
bool IsOverride = false;
@@ -267,7 +271,7 @@ struct CompletionCandidate {
// This could break #include insertion.
return hash_combine(
(IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
-headerToInsertIfNotPresent().getValueOr(""));
+headerToInsertIfAllowed().getValueOr(""));
default:
return 0;
}
@@ -281,11 +285,12 @@ struct CompletionCandidate {
llvm::raw_svector_ostream OS(Scratch);
D->printQualifiedName(OS);
}
-return hash_combine(Scratch, headerToInsertIfNotPresent().getValueOr(""));
+return hash_combine(Scratch, headerToInsertIfAllowed().getValueOr(""));
}
- llvm::Optional headerToInsertIfNotPresent() const {
-if (!IndexResult || IndexResult->IncludeHeader.empty())
+ // The best header to include if include insertion is allowed.
+ llvm::Optional headerToInsertIfAllowed() const {
+if (RankedIncludeHeaders.empty())
return llvm::None;
if (SemaResult && SemaResult->Declaration) {
// Avoid inserting new #include if the declaration is found in the
current
@@ -295,7 +300,7 @@ struct CompletionCandidate {
if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc(
return llvm::None;
}
-return IndexResult->IncludeHeader;
+return RankedIncludeHeaders[0];
}
using Bundle = llvm::SmallVector;
@@ -358,31 +363,41 @@ struct CodeCompletionBuilder {
if (Completion.Name.empty())
Completion.Name = C.IndexResult->Name;
}
-if (auto Inserted = C.headerToInsertIfNotPresent()) {
- // Turn absolute path into a literal string that can be #included.
- auto Include = [&]() -> Expected> {
-auto ResolvedDeclaring =
-toHeaderFile(C.IndexResult->CanonicalDeclaration.FileURI,
FileName);
-if (!ResolvedDeclaring)
- return ResolvedDeclaring.takeError();
-auto ResolvedInserted = toHeaderFile(*Inserted, FileName);
-if (!ResolvedInserted)
- return ResolvedInserted.takeError();
-return std::make_pair(Includes.calculateIncludePath(*ResolvedDeclaring,
-*ResolvedInserted),
- Includes.shouldInsertInclude(*ResolvedDeclaring,
-
[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.
ioeric updated this revision to Diff 163678.
ioeric added a comment.
- Document limitation for overload bundling.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51291
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -54,7 +54,13 @@
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
-MATCHER_P(IncludeHeader, P, "") { return arg.IncludeHeader == P; }
+MATCHER_P(IncludeHeader, P, "") {
+ return (arg.IncludeHeaders.size() == 1) &&
+ (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
MATCHER_P(DeclRange, Pos, "") {
return std::tie(arg.CanonicalDeclaration.Start.Line,
arg.CanonicalDeclaration.Start.Column,
@@ -760,6 +766,11 @@
IsIndexedForCodeCompletion:true
Documentation:'Foo doc'
ReturnType:'int'
+IncludeHeaders:
+ - Header:'include1'
+References:7
+ - Header:'include2'
+References:3
...
)";
const std::string YAML2 = R"(
@@ -791,6 +802,10 @@
Doc("Foo doc"), ReturnType("int"),
DeclURI("file:///path/foo.h"),
ForCodeCompletion(true;
+ auto = *Symbols1.begin();
+ EXPECT_THAT(Sym1.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
+ IncludeHeaderWithRef("include2", 3u)));
auto Symbols2 = symbolsFromYAML(YAML2);
EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
QName("clang::Foo2"), Labeled("Foo2-sig"),
@@ -812,9 +827,10 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
- IncludeHeader(TestHeaderURI;
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI;
+ EXPECT_THAT(Symbols.begin()->IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
}
#ifndef _WIN32
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -306,6 +306,46 @@
FileURI("unittest:///test2.cc";
}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
+
+TEST(MergeTest, MergeIncludesOnDifferentDefinitions) {
+ Symbol L, R;
+ L.Name = "left";
+ R.Name = "right";
+ L.ID = R.ID = SymbolID("hello");
+ L.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("new", 1);
+
+ // Both have no definition.
+ Symbol M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Only merge references of the same includes but do not merge new #includes.
+ L.Definition.FileURI = "file:/left.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u)));
+
+ // Definitions are the same.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Definitions are different.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===
---
[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.
ioeric updated this revision to Diff 163677.
ioeric added a comment.
- Rebase
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51291
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -54,7 +54,13 @@
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
-MATCHER_P(IncludeHeader, P, "") { return arg.IncludeHeader == P; }
+MATCHER_P(IncludeHeader, P, "") {
+ return (arg.IncludeHeaders.size() == 1) &&
+ (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
MATCHER_P(DeclRange, Pos, "") {
return std::tie(arg.CanonicalDeclaration.Start.Line,
arg.CanonicalDeclaration.Start.Column,
@@ -760,6 +766,11 @@
IsIndexedForCodeCompletion:true
Documentation:'Foo doc'
ReturnType:'int'
+IncludeHeaders:
+ - Header:'include1'
+References:7
+ - Header:'include2'
+References:3
...
)";
const std::string YAML2 = R"(
@@ -791,6 +802,10 @@
Doc("Foo doc"), ReturnType("int"),
DeclURI("file:///path/foo.h"),
ForCodeCompletion(true;
+ auto = *Symbols1.begin();
+ EXPECT_THAT(Sym1.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
+ IncludeHeaderWithRef("include2", 3u)));
auto Symbols2 = symbolsFromYAML(YAML2);
EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
QName("clang::Foo2"), Labeled("Foo2-sig"),
@@ -812,9 +827,10 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
- IncludeHeader(TestHeaderURI;
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI;
+ EXPECT_THAT(Symbols.begin()->IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
}
#ifndef _WIN32
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -306,6 +306,46 @@
FileURI("unittest:///test2.cc";
}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
+
+TEST(MergeTest, MergeIncludesOnDifferentDefinitions) {
+ Symbol L, R;
+ L.Name = "left";
+ R.Name = "right";
+ L.ID = R.ID = SymbolID("hello");
+ L.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("new", 1);
+
+ // Both have no definition.
+ Symbol M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Only merge references of the same includes but do not merge new #includes.
+ L.Definition.FileURI = "file:/left.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u)));
+
+ // Definitions are the same.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Definitions are different.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++
[PATCH] D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed
miyuki added a comment. ping https://reviews.llvm.org/D50507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.
ilya-biryukov added a comment.
Thanks, the interface and implementation look good!
Last drop of nits and suggestions for code simplification.
Comment at: include/clang/Basic/VirtualFileSystem.h:359
+ public:
+ /// Add a HardLink to a File.
+ /// The To path must be an existing file or a hardlink. The From file must
not
usaxena95 wrote:
> ilya-biryukov wrote:
> > Maybe document that it does nothing for directories?
> > Also, any reason to not support directories at this point? The limitation
> > seem artificial at this point.
> Links to directories cannot be added with the current logic. Resolving paths
> and iterating directories becomes non trivial. Current implementation
> supports the use case of "file with multiple names" which is mostly
> sufficient to mimic the compilation.
You're right. E.g. one potential trouble is cycles in the directory tree.
Let's leave them out, directory links do seem like some hard work and we don't
have a use-case for them.
Comment at: include/clang/Basic/VirtualFileSystem.h:364
+ /// The To path must be an existing file or a hardlink. The From file must
not
+ /// have been added before. The From Node is added as an InMemoryHardLink
+ /// which points to the resolved file of To Node. The From path or the To
Path
InMemoryHardLink is a private implementation detail. Maybe remove it from the
docs of the public API?
Comment at: include/clang/Basic/VirtualFileSystem.h:369
+ /// successfully created, false otherwise.
+ bool addHardLink(const Twine , const Twine ,
+ Optional User = None,
Maybe expand a little on what a "hard link" is in our implementation?
Specifically, the important bits seem to be:
They are not intended to be fully equivalent to hard links of the classical
filesystems, despite the name.
Both the original file and a hard link have the same UniqueID returned in their
stats, share the same buffer, etc.
There is no way to distinguish hard links and the original files after they
were added.
(this is already in the docs) Only links to files are supported, directories
cannot be linked.
Comment at: include/clang/Basic/VirtualFileSystem.h:370
+ bool addHardLink(const Twine , const Twine ,
+ Optional User = None,
+ Optional Group = None,
Maybe remove these defaulted parameters? They're not used and we'll probably
have confusing semantics even if we add support for them (since our hard links
don't have their own status, and merely return the status of the original file).
WDYT?
Comment at: lib/Basic/VirtualFileSystem.cpp:476
InMemoryNodeKind Kind;
-
-protected:
- /// Return Stat. This should only be used for internal/debugging use. When
- /// clients wants the Status of this node, they should use
- /// \p getStatus(StringRef).
- const Status () const { return Stat; }
+ string FileName;
Please use std::string, we usually don't leave out the std:: namespace in LLVM.
Comment at: lib/Basic/VirtualFileSystem.cpp:523
+ : InMemoryNode(Path.str(), IME_HardLink), ResolvedFile(ResolvedFile) {}
+ const InMemoryFile *getResolvedFile() const { return }
+
Maybe return a reference here too? To indicate this can't be null.
Comment at: lib/Basic/VirtualFileSystem.cpp:653
+ // Cannot create HardLink from a directory.
+ if (HardLinkTarget && (!isa(HardLinkTarget) || Buffer))
+return false;
HardLinkTarget can only be `InMemoryFile`, so maybe change the type of the
parameter to `const InMemoryFile*`?
Clients will be responsible for making sure the target is actually a file, but
that seems fine, they already have to resolve the path to the target file.
Comment at: lib/Basic/VirtualFileSystem.cpp:665
// End of the path, create a new file or directory.
Status Stat(P.str(), getNextVirtualUniqueID(),
llvm::sys::toTimePoint(ModificationTime), ResolvedUser,
We don't use `Status` for links, maybe move creation of `Status` into the
branch that handles files?
This would allow to assert `Buffer` is not null too
Comment at: lib/Basic/VirtualFileSystem.cpp:690
getNextVirtualUniqueID(), llvm::sys::toTimePoint(ModificationTime),
- ResolvedUser, ResolvedGroup, Buffer->getBufferSize(),
- sys::fs::file_type::directory_file, NewDirectoryPerms);
+ ResolvedUser, ResolvedGroup, 0, sys::fs::file_type::directory_file,
+ NewDirectoryPerms);
Oh, wow, this used to set the size of the created parent directories to the
size of the buffer we're creating. I guess nobody looks at it anyway, so we
were never hitting any problems because of this.
Good catch, thanks for fixing this.
Comment
[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters
baloghadamsoftware marked 3 inline comments as done.
baloghadamsoftware added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
+def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
+ HelpText<"Check for use of iterators of different containers where iterators
of the same container are expected">,
whisperity wrote:
> Is there any particular order entries of this file should be in? Seems to be
> broken now, but I guess when this patch comes up to the top of the stack it
> shall be fixed at a rebase.
Alphabetical order. Now it seems to be OK.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:382
}
+ } else if (!isa()) {
+// The main purpose of iterators is to abstract away from different
baloghadamsoftware wrote:
> a.sidorin wrote:
> > The function becomes > 100 lines long. Should we refactor this check into a
> > separate function to improve readability?
> Yes, I think so this would be a good idea. Should I do it now?
I propose to refactor it later in a separate patch.
https://reviews.llvm.org/D32845
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters
baloghadamsoftware updated this revision to Diff 163671.
baloghadamsoftware added a comment.
Minor changes.
https://reviews.llvm.org/D32845
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx.h
test/Analysis/invalidated-iterator.cpp
test/Analysis/mismatched-iterator.cpp
Index: test/Analysis/mismatched-iterator.cpp
===
--- /dev/null
+++ test/Analysis/mismatched-iterator.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.MismatchedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.MismatchedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void good_find(std::vector , int n) {
+ std::find(v.cbegin(), v.cend(), n); // no-warning
+}
+
+void good_find_first_of(std::vector , std::vector ) {
+ std::find_first_of(v1.cbegin(), v1.cend(), v2.cbegin(), v2.cend()); // no-warning
+}
+
+void good_copy(std::vector , std::vector , int n) {
+ std::copy(v1.cbegin(), v1.cend(), v2.begin()); // no-warning
+}
+
+void bad_find(std::vector , std::vector , int n) {
+ std::find(v1.cbegin(), v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
+
+void bad_find_first_of(std::vector , std::vector ) {
+ std::find_first_of(v1.cbegin(), v2.cend(), v2.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
+ std::find_first_of(v1.cbegin(), v1.cend(), v2.cbegin(), v1.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
+}
Index: test/Analysis/invalidated-iterator.cpp
===
--- test/Analysis/invalidated-iterator.cpp
+++ test/Analysis/invalidated-iterator.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-relational-comparison-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-relational-comparison-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
#include "Inputs/system-header-simulator-cxx.h"
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -642,6 +642,12 @@
template
InputIterator find(InputIterator first, InputIterator last, const T );
+ template
+ ForwardIterator1 find_first_of(ForwardIterator1 first1,
+ ForwardIterator1 last1,
+ ForwardIterator2 first2,
+ ForwardIterator2 last2);
+
template
OutputIterator copy(InputIterator first, InputIterator last,
OutputIterator result);
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -198,6 +198,7 @@
eval::Assume> {
std::unique_ptr OutOfRangeBugType;
+ std::unique_ptr MismatchedBugType;
std::unique_ptr InvalidatedBugType;
void handleComparison(CheckerContext , const SVal , const SVal ,
@@ -221,16 +222,23 @@
void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
const SVal , const SVal ,
const SVal ) const;
+ void verifyMatch(CheckerContext , const SVal ,
+ const SVal ) const;
+
void reportOutOfRangeBug(const StringRef , const SVal ,
CheckerContext , ExplodedNode *ErrNode) const;
+ void reportMismatchedBug(const StringRef , const SVal ,
+ const SVal ,
[PATCH] D51481: [clangd] Implement proximity path boosting for Dex
kbobyrev updated this revision to Diff 163667.
kbobyrev marked 2 inline comments as done.
kbobyrev edited the summary of this revision.
kbobyrev added a comment.
Rebase on top of `master`, s/Path/PathURI/g to be more explicit about the token
contents and origin.
https://reviews.llvm.org/D51481
Files:
clang-tools-extra/;
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/index/dex/DexIndex.cpp
clang-tools-extra/clangd/index/dex/DexIndex.h
clang-tools-extra/clangd/index/dex/Token.cpp
clang-tools-extra/clangd/index/dex/Token.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/unittests/clangd/DexIndexTests.cpp
clang-tools-extra/unittests/clangd/TestFS.cpp
clang-tools-extra/unittests/clangd/TestFS.h
Index: clang-tools-extra/unittests/clangd/TestFS.h
===
--- clang-tools-extra/unittests/clangd/TestFS.h
+++ clang-tools-extra/unittests/clangd/TestFS.h
@@ -59,7 +59,7 @@
};
// Returns an absolute (fake) test directory for this OS.
-const char *testRoot();
+std::string testRoot();
// Returns a suitable absolute path for this OS.
std::string testPath(PathRef File);
Index: clang-tools-extra/unittests/clangd/TestFS.cpp
===
--- clang-tools-extra/unittests/clangd/TestFS.cpp
+++ clang-tools-extra/unittests/clangd/TestFS.cpp
@@ -64,7 +64,7 @@
FileName, std::move(CommandLine), "")};
}
-const char *testRoot() {
+std::string testRoot() {
#ifdef _WIN32
return "C:\\clangd-test";
#else
Index: clang-tools-extra/unittests/clangd/DexIndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexIndexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexIndexTests.cpp
@@ -7,6 +7,8 @@
//
//===--===//
+#include "FuzzyMatch.h"
+#include "TestFS.h"
#include "TestIndex.h"
#include "index/Index.h"
#include "index/Merge.h"
@@ -29,6 +31,8 @@
namespace dex {
namespace {
+std::vector URISchemes = {"unittest"};
+
std::vector consumeIDs(Iterator ) {
auto IDAndScore = consume(It);
std::vector IDs(IDAndScore.size());
@@ -325,14 +329,33 @@
}
testing::Matcher>
-trigramsAre(std::initializer_list Trigrams) {
+tokensAre(std::initializer_list Strings, Token::Kind Kind) {
std::vector Tokens;
- for (const auto : Trigrams) {
-Tokens.push_back(Token(Token::Kind::Trigram, Symbols));
+ for (const auto : Strings) {
+Tokens.push_back(Token(Kind, TokenData));
}
return testing::UnorderedElementsAreArray(Tokens);
}
+testing::Matcher>
+trigramsAre(std::initializer_list Trigrams) {
+ return tokensAre(Trigrams, Token::Kind::Trigram);
+}
+
+testing::Matcher>
+pathsAre(std::initializer_list Paths) {
+ return tokensAre(Paths, Token::Kind::PathURI);
+}
+
+testing::Matcher>> proximityPathsAre(
+std::initializer_list> ProximityPaths) {
+ std::vector> Result;
+ for (const auto : ProximityPaths) {
+Result.push_back({Token(Token::Kind::PathURI, P.first), P.second});
+ }
+ return testing::UnorderedElementsAreArray(Result);
+}
+
TEST(DexIndexTrigrams, IdentifierTrigrams) {
EXPECT_THAT(generateIdentifierTrigrams("X86"),
trigramsAre({"x86", "x$$", "x8$"}));
@@ -407,8 +430,28 @@
"hij", "ijk", "jkl", "klm"}));
}
+TEST(DexSearchTokens, SymbolPath) {
+ EXPECT_THAT(generateProximityPaths(
+ "unittest:///clang-tools-extra/clangd/index/dex/Token.h"),
+ pathsAre({"unittest:///clang-tools-extra/clangd/index/dex",
+"unittest:///clang-tools-extra/clangd/index",
+"unittest:///clang-tools-extra/clangd",
+"unittest:///clang-tools-extra",
+"unittest:///"}));
+}
+
+TEST(DexSearchTokens, QueryProximityDistances) {
+ EXPECT_THAT(
+ generateQueryProximityPaths(testRoot() + "/a/b/c/d/e/f/g.h", URISchemes),
+ proximityPathsAre({{"unittest:///a/b/c/d/e/f", 0},
+ {"unittest:///a/b/c/d/e", 1},
+ {"unittest:///a/b/c/d", 2},
+ {"unittest:///a/b/c", 3},
+ {"unittest:///a/b", 4}}));
+}
+
TEST(DexIndex, Lookup) {
- DexIndex I;
+ DexIndex I(URISchemes);
I.build(generateSymbols({"ns::abc", "ns::xyz"}));
EXPECT_THAT(lookup(I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc"));
EXPECT_THAT(lookup(I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}),
@@ -419,9 +462,10 @@
}
TEST(DexIndex, FuzzyFind) {
- DexIndex Index;
+ DexIndex Index(URISchemes);
Index.build(generateSymbols({"ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC",
- "other::ABC", "other::A"}));
+ "other::ABC", "other::A"})
+ );
FuzzyFindRequest Req;
Req.Query = "ABC";
Req.Scopes =
[PATCH] D51481: [clangd] Implement proximity path boosting for Dex
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:36
+Result.push_back(PathToken);
+ }
return Result;
nit: no need for braces. [llvm-style]
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:126
+std::vector> BoostingIterators;
+// This should generate proximity path boosting iterators for each
different
+// URI Scheme the Index is aware of.
I thought we wanted to take the first scheme that works, given that
`URISchemes` is sorted by preference?
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:137
+ BoostingIterators.push_back(
+ createBoost(create(It->second), PARENTS_TO_BOOST + 1 -
P.second));
+ }
`PARENTS_TO_BOOST + 1 - P.second` seems to be too much boosting. We should
align the boosting function with the one we use in clangd/Quality.cpp, e.g.
proximity score should be would be in the range [0, 1], and we can boost by
`1+proximity`.
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:161
+// Sort items using boosting score as the key.
+// FIXME(kbobyrev): Consider merging this stage with the next one? This is
nit: in general, this should be a combination of relevance score (e.g.
proximity) and quality score (e.g. #references).
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:169
+ const std::pair ) {
+return SymbolQuality.find((*Symbols)[LHS.first])->second *
+ LHS.second >
nit: `SymbolQuality[(*Symbols)[LHS.first]]`?
For performance reason, could we instead make `SymbolQuality` a vector indexed
by `DocID`?
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:42
public:
+ DexIndex(llvm::ArrayRef URISchemes) : URISchemes(URISchemes) {}
+
nit: explicit?
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:42
public:
+ DexIndex(llvm::ArrayRef URISchemes) : URISchemes(URISchemes) {}
+
ioeric wrote:
> nit: explicit?
Add documentation about `URISchemes`?
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:18
+
+std::vector generateProximityPaths(llvm::StringRef URIPath,
+ size_t Limit) {
Add `FIXME` mentioning that the parsing and encoding here are not necessary and
could potentially be optimized.
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:28
+ "Non-empty argument of generateProximityPaths() should be a valid URI.");
+ const auto Scheme = ParsedURI.get().scheme();
+ const auto Authority = ParsedURI.get().authority();
nit: `ParsedURI->scheme();`
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:32
+ // Get parent directory of the file with symbol declaration.
+ Path = llvm::sys::path::parent_path(Path);
+ while (!Path.empty() && Limit--) {
I think you need to set `posix` style explicitly here. On windows, the
separator is '/'.
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:32
+ // Get parent directory of the file with symbol declaration.
+ Path = llvm::sys::path::parent_path(Path);
+ while (!Path.empty() && Limit--) {
ioeric wrote:
> I think you need to set `posix` style explicitly here. On windows, the
> separator is '/'.
I think the original path should also be used as a proximity path.
Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:51
+// should be just skipped.
+if (!static_cast(URI)) {
+ // Ignore the error.
You could use `llvm::consumeError(URI.takeError())`.
Comment at: clang-tools-extra/clangd/index/dex/Token.h:45
+ // FIXME(kbobyrev): Storing Data hash would be more efficient than storing
raw
+ // strings.
enum class Kind {
nit: For example, (URI is an good example I think)
Comment at: clang-tools-extra/clangd/index/dex/Token.h:58
+///
+/// Data stores URI the parent directory of symbol declaration location.
+///
Do we also consider the original URI as a proximity path?
Comment at: clang-tools-extra/clangd/index/dex/Token.h:94
+///
+/// When Limit is set, returns no more than Limit tokens.
+std::vector
nit: also explain how Limit is used? e.g. closer parent directories are
preferred?
Comment at: clang-tools-extra/clangd/index/dex/Token.h:96
+std::vector
+generateProximityPaths(llvm::StringRef Path,
+ size_t Limit = std::numeric_limits::max());
As we are assuming that `Path` is an URI. We should be more explicit in the
function names.
[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1307
+ (Style.BraceWrapping.BeforeLambdaBody && Current.Next != nullptr &&
+Current.Next->is(TT_LambdaLSquare)));
State.Stack.back().IsInsideObjCArrayLiteral =
Wawha wrote:
> klimek wrote:
> > klimek wrote:
> > > I think I misunderstood some of this the first time around (and thanks
> > > for bearing with me :) - iiuc we want to break for Allman even when
> > > there's only one nested block. I think I'll need to take another look
> > > when I'm back from vacation, unless Daniel or somebody else finds time
> > > before then (will be back in 1.5 weeks)
> > So, HasMultipleNestedBlocks should only be true if there are multiple
> > nested blocks.
> > What tests break if you remove this change to HasMultipleNestedBlocks?
> All cases with only one lambda in parameter are break. The Lambda body with
> be put on the same line as the function and aligned with [] instead of
> putting the body [] on another line with just one simple indentation.
>
> So if restore the line to :
>
> ```
> State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1;
> ```
>
> Here some cases.
> FormatTest.cpp, ligne 11412:
>
> ```
> // With my modification
> FunctionWithOneParam(
> []()
> {
> // A cool function...
> return 43;
> });
>
> // Without my modification
> FunctionWithOneParam([]()
> {
>// A cool function...
>return 43;
> });
> ```
> The "{" block of the lambda will be aligned on the "[" depending of the
> function name length.
>
>
> You have the same case with multiple lambdas (FormatTest.cpp, ligne 11433):
>
> ```
> // With my modification
> TwoNestedLambdas(
> []()
> {
> return Call(
> []()
> {
> return 17;
> });
> });
>
> // Without my modification
> TwoNestedLambdas([]()
> {
>return Call([]()
>{
> return 17;
>});
> });
> ```
Do we want to always break before lambdas in Allman style?
Repository:
rC Clang
https://reviews.llvm.org/D44609
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:57
+ functionDecl(
+ isDefinition(),
+ unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)),
hokein wrote:
> any reason why we restrict to definitions only? I think we can consider
> declarations too.
I restricted the check to function definitions because function declarations
are sometimes associated with functions outside the control of the author. I
have personally observed unfortunate cases where functions declared in the iOS
SDK had incorrect—or seemingly incorrect—availability attributes that caused
fatal dyld assertions during application startup. The check currently intends
to avoid flagging function declarations because of the rare circumstances where
an inflexible function declaration without a corresponding function definition
is required.
I have added a comment explaining why only function definitions are flagged.
I am still open to discussion though.
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:109
+TEST(ObjCFunctionNaming, AllowedStaticFunctionName) {
+ std::vector Errors;
hokein wrote:
> nit: we don't need unittest for the check here, as it is well covered in the
> littest.
Removed the unit tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51575
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions
stephanemoore updated this revision to Diff 163656.
stephanemoore marked an inline comment as done.
stephanemoore added a comment.
Updated with changes:
- Removed unit tests as other tests have been indicated to provide adequate
coverage.
- Added a comment explaining why only function definitions are evaluated.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51575
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/FunctionNamingCheck.cpp
clang-tidy/google/FunctionNamingCheck.h
clang-tidy/google/GoogleTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/google-objc-function-naming.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/google-objc-function-naming.m
Index: test/clang-tidy/google-objc-function-naming.m
===
--- /dev/null
+++ test/clang-tidy/google-objc-function-naming.m
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s google-objc-function-naming %t
+
+typedef _Bool bool;
+
+static bool ispositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'ispositive' not using function naming conventions described by Google Objective-C style guide
+
+static bool is_positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'is_positive' not using function naming conventions described by Google Objective-C style guide
+
+static bool isPositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'isPositive' not using function naming conventions described by Google Objective-C style guide
+
+static bool Is_Positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'Is_Positive' not using function naming conventions described by Google Objective-C style guide
+
+static bool IsPositive(int a) { return a > 0; }
+
+static const char *md5(const char *str) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: function name 'md5' not using function naming conventions described by Google Objective-C style guide
+
+static const char *MD5(const char *str) { return 0; }
+
+static const char *URL(void) { return "https://clang.llvm.org/;; }
+
+static const char *DEFURL(void) { return "https://clang.llvm.org/;; }
+
+static const char *DEFFooURL(void) { return "https://clang.llvm.org/;; }
+
+static const char *StringFromNSString(id str) { return ""; }
+
+bool ispalindrome(const char *str);
+
+void ABLog_String(const char *str) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ABLog_String' not using function naming conventions described by Google Objective-C style guide
+
+void ABLogString(const char *str) {}
+
+const char *ABURL(void) { return "https://clang.llvm.org/;; }
+
+const char *ABFooURL(void) { return "https://clang.llvm.org/;; }
+
+int main(int argc, const char **argv) { return 0; }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -118,6 +118,7 @@
google-explicit-constructor
google-global-names-in-headers
google-objc-avoid-throwing-exception
+ google-objc-function-naming
google-objc-global-variable-declaration
google-readability-braces-around-statements (redirects to readability-braces-around-statements)
google-readability-casting
Index: docs/clang-tidy/checks/google-objc-function-naming.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/google-objc-function-naming.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - google-objc-function-naming
+
+google-objc-function-naming
+===
+
+Finds function definitions in Objective-C files that do not follow the pattern
+described in the Google Objective-C Style Guide.
+
+The corresponding style guide rule can be found here:
+https://google.github.io/styleguide/objcguide.html#function-names
+
+All function names should be in upper camel case. Functions whose storage class
+is not static should have an appropriate prefix.
+
+The following code sample does not follow this pattern:
+
+.. code-block:: objc
+
+ static bool is_positive(int i) { return i > 0; }
+ bool IsNegative(int i) { return i < 0; }
+
+The sample above might be corrected to the following code:
+
+.. code-block:: objc
+
+ static bool IsPositive(int i) { return i > 0; }
+ bool *ABCIsNegative(int i) { return i < 0; }
+
+The check does not currently recommend any fixes.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -93,6 +93,12 @@
Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
``absl::StrAppend()`` should be used instead.
+- New :doc:`google-objc-function-naming
+ ` check.
+
+ Checks that function names in function definitions comply with the naming
+ conventions
[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions
hokein added a comment.
Nice! looks mostly good to me.
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:57
+ functionDecl(
+ isDefinition(),
+ unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)),
any reason why we restrict to definitions only? I think we can consider
declarations too.
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:109
+TEST(ObjCFunctionNaming, AllowedStaticFunctionName) {
+ std::vector Errors;
nit: we don't need unittest for the check here, as it is well covered in the
littest.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51575
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50958: [clangd] Implement findReferences function
hokein updated this revision to Diff 163654.
hokein edited the summary of this revision.
hokein added a comment.
Rebase
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50958
Files:
clangd/XRefs.cpp
clangd/XRefs.h
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -26,6 +26,7 @@
using namespace llvm;
namespace {
+using testing::_;
using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
@@ -1068,6 +1069,179 @@
ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
}
+TEST(FindReferences, AllWithoutIndex) {
+ const char *Tests[] = {
+ R"cpp(// Local variable
+int main() {
+ int $foo[[foo]];
+ $foo[[^foo]] = 2;
+ int test1 = $foo[[foo]];
+}
+ )cpp",
+
+ R"cpp(// Struct
+namespace ns1 {
+struct $foo[[Foo]] {};
+} // namespace ns1
+int main() {
+ ns1::$foo[[Fo^o]]* Params;
+}
+ )cpp",
+
+ R"cpp(// Function
+int $foo[[foo]](int) {}
+int main() {
+ auto *X = &$foo[[^foo]];
+ $foo[[foo]](42)
+}
+ )cpp",
+
+ R"cpp(// Field
+struct Foo {
+ int $foo[[foo]];
+ Foo() : $foo[[foo]](0) {}
+};
+int main() {
+ Foo f;
+ f.$foo[[f^oo]] = 1;
+}
+ )cpp",
+
+ R"cpp(// Method call
+struct Foo { int [[foo]](); };
+int Foo::[[foo]]() {}
+int main() {
+ Foo f;
+ f.^foo();
+}
+ )cpp",
+
+ R"cpp(// Typedef
+typedef int $foo[[Foo]];
+int main() {
+ $foo[[^Foo]] bar;
+}
+ )cpp",
+
+ R"cpp(// Namespace
+namespace $foo[[ns]] {
+struct Foo {};
+} // namespace ns
+int main() { $foo[[^ns]]::Foo foo; }
+ )cpp",
+ };
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+std::vector> ExpectedLocations;
+for (const auto : T.ranges("foo"))
+ ExpectedLocations.push_back(RangeIs(R));
+EXPECT_THAT(findReferences(AST, T.point()),
+ElementsAreArray(ExpectedLocations))
+<< Test;
+ }
+}
+
+class MockIndex : public SymbolIndex {
+public:
+ MOCK_CONST_METHOD2(fuzzyFind, bool(const FuzzyFindRequest &,
+ llvm::function_ref));
+ MOCK_CONST_METHOD2(lookup, void(const LookupRequest &,
+ llvm::function_ref));
+ MOCK_CONST_METHOD2(findOccurrences,
+ void(const OccurrencesRequest &,
+ llvm::function_ref));
+ MOCK_CONST_METHOD0(estimateMemoryUsage, size_t());
+};
+
+TEST(FindReferences, QueryIndex) {
+ const char *Tests[] = {
+ // Refers to symbols from headers.
+ R"cpp(
+int main() {
+ F^oo foo;
+}
+ )cpp",
+ R"cpp(
+int main() {
+ f^unc();
+}
+ )cpp",
+ R"cpp(
+int main() {
+ return I^NT;
+}
+ )cpp",
+
+ // These are cases of file-local but not function-local symbols, we still
+ // query the index.
+ R"cpp(
+void MyF^unc() {}
+ )cpp",
+
+ R"cpp(
+int My^Int = 2;
+ )cpp",
+ };
+
+ TestTU TU;
+ TU.HeaderCode = R"(
+ class Foo {};
+ static const int INT = 3;
+ inline void func() {};
+ )";
+ MockIndex Index;
+ for (const char *Test : Tests) {
+Annotations T(Test);
+TU.Code = T.code();
+auto AST = TU.build();
+EXPECT_CALL(Index, findOccurrences(_, _));
+findReferences(AST, T.point(), );
+ }
+}
+
+TEST(FindReferences, DontQueryIndex) {
+ // Don't query index for function-local symbols.
+ const char *Tests[] = {
+ R"cpp(// Local variable in function body
+int main() {
+ int $foo[[foo]];
+ $foo[[^foo]] = 2;
+}
+ )cpp",
+
+ R"cpp(// function parameter
+int f(int fo^o) {
+}
+ )cpp",
+
+ R"cpp(// function parameter in lambda
+int f(int foo) {
+ auto func = [](int a, int b) {
+return ^a = 2;
+ };
+}
+ )cpp",
+
+ R"cpp(// capture in lambda
+int f(int foo) {
+ int A;
+ auto func = [](int a, int b) {
+return a = ^A;
+ };
+}
+ )cpp",
+ };
+
+ MockIndex Index;
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+EXPECT_CALL(Index, findOccurrences(_, _)).Times(0);
+findReferences(AST, T.point(), );
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -34,6 +34,9
