Re: [PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
I've sent r325482 which (hopefully) fixes the failure. I'll keep an eye on the bot to make sure this is fixed ASAP. Sorry about the inconvenience! o Eric On Mon, Feb 19, 2018 at 11:39 AM Carlos Alberto Enciso via Phabricator < revi...@reviews.llvm.org> wrote: > CarlosAlbertoEnciso added a subscriber: aheejin. > CarlosAlbertoEnciso added a comment. > > Thanks @aheejin > > > Repository: > rL LLVM > > https://reviews.llvm.org/D42640 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
CarlosAlbertoEnciso added a subscriber: aheejin. CarlosAlbertoEnciso added a comment. Thanks @aheejin Repository: rL LLVM https://reviews.llvm.org/D42640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
Hi Carlos, did this fail on a Windows build bot? Would you mind pointing me to the broken test? Thanks! On Fri, Feb 16, 2018 at 4:23 PM Carlos Alberto Enciso via Phabricator < revi...@reviews.llvm.org> wrote: > CarlosAlbertoEnciso added a comment. > > Hi @ioeric: > > Just to let you know that your submission seems to break a Windows test. > > FAIL: Extra Tools Unit Tests :: > clangd/Checking/./ClangdTests.exe/SymbolCollectorTest.IWYUPragma (15474 of > 36599) > > Thanks > > > Repository: > rL LLVM > > https://reviews.llvm.org/D42640 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
CarlosAlbertoEnciso added a comment. Hi @ioeric: Just to let you know that your submission seems to break a Windows test. FAIL: Extra Tools Unit Tests :: clangd/Checking/./ClangdTests.exe/SymbolCollectorTest.IWYUPragma (15474 of 36599) Thanks Repository: rL LLVM https://reviews.llvm.org/D42640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE325343: [clangd] collect symbol #include insert
#include in global code completion. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42640?vs=134600=134603#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/completion-snippets.test
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,9 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(DeclRange, Offsets, "") {
return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
arg.CanonicalDeclaration.EndOffset == Offsets.second;
@@ -62,41 +65,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(testPath("symbol.h")),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(testPath("symbol.h")),
TestFileName(testPath("symbol.cc")) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", "-include",
@@ -117,12 +141,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
TEST_F(SymbolCollectorTest,
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325343: [clangd] collect symbol #include insert
#include in global code completion. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D42640
Files:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Headers.cpp
clang-tools-extra/trunk/clangd/Headers.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.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/SymbolCollector.h
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/completion-snippets.test
clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
clang-tools-extra/trunk/test/clangd/initialize-params.test
clang-tools-extra/trunk/test/clangd/insert-include.test
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/test/clangd/insert-include.test
===
--- clang-tools-extra/trunk/test/clangd/insert-include.test
+++ clang-tools-extra/trunk/test/clangd/insert-include.test
@@ -0,0 +1,36 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void f() {}"}}}
+---
+{"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{"arguments":[{"header":"","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}}
+# CHECK:"id": 3,
+# CHECK-NEXT:"jsonrpc": "2.0",
+# CHECK-NEXT:"result": "Inserted header "
+# CHECK-NEXT: }
+# CHECK:"method": "workspace/applyEdit",
+# CHECK-NEXT:"params": {
+# CHECK-NEXT: "edit": {
+# CHECK-NEXT:"changes": {
+# CHECK-NEXT: "file:///clangd-test/main.cpp": [
+# CHECK-NEXT:{
+# CHECK-NEXT: "newText": "#include \n",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+# CHECK-NEXT: ]
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -24,7 +24,8 @@
# CHECK-NEXT: "documentRangeFormattingProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.insertInclude"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "renameProvider": true,
Index: clang-tools-extra/trunk/test/clangd/completion-snippets.test
===
--- clang-tools-extra/trunk/test/clangd/completion-snippets.test
+++ clang-tools-extra/trunk/test/clangd/completion-snippets.test
@@ -28,9 +28,7 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT:"isIncomplete": {{.*}}
# CHECK-NEXT:"items": [
-# CHECK-NEXT:{
-# CHECK-NEXT: "detail": "int",
-# CHECK-NEXT: "filterText": "func_with_args",
+# CHECK: "filterText": "func_with_args",
# CHECK-NEXT: "insertText": "func_with_args(${1:int a}, ${2:int b})",
# CHECK-NEXT:
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 134600.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- Merged with origin/master
- Addressed review comments; removed .inc handling.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/completion-snippets.test
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,9 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(DeclRange, Offsets, "") {
return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
arg.CanonicalDeclaration.EndOffset == Offsets.second;
@@ -62,41 +65,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(testPath("symbol.h")),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(testPath("symbol.h")),
TestFileName(testPath("symbol.cc")) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", "-include",
@@ -117,12 +141,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
TEST_F(SymbolCollectorTest, CollectSymbols) {
@@ -554,6 +580,46 @@
QName("clang::Foo2")));
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric added a comment.
Thank you for reviewing this!
Comment at: clangd/index/CanonicalIncludes.h:52
+ /// a canonical header name.
+ llvm::StringRef mapHeader(llvm::StringRef Header) const;
+
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > So I'm a bit concerned this is too narrow an interface, and we really
> > > want to deal with SourceLocation here which would give us the include
> > > stack.
> > >
> > > Evidence #1: .inc handling really is the same job, but because this class
> > > has a single-file interface, we have to push it into the caller.
> > > Evidence #2: I was thinking about a more... principled way of determining
> > > system headers. One option would be to have a whitelist of public
> > > headers, and walk up the include stack to see if you hit one. (I'm not
> > > 100% sure this would work, but still...) This isn't implementable with
> > > the current interface.
> > >
> > > One compromise would be to pass in a stack or something.
> > > Efficiency doesn't really matter because the caller can cache based on
> > > the top element.
> > > Evidence #1: .inc handling really is the same job, but because this class
> > > has a single-file interface, we have to push it into the caller.
> > I think this would depend on how you define the scope of this class. `.inc`
> > handling is a subtle case that I'm a bit hesitated to build into the
> > interface here.
> >
> > > Evidence #2:
> > This is actually very similar to how the hardcoded mapping was generated. I
> > had a tool that examined include stacks for a library (e.g. STL) and
> > applied a similar heuristic - treat the last header in the include stack
> > within the library boundary as the "exporting" public header for a leaf
> > include header, if there is no other public header that has shorter
> > distance to that include. For example, if we see a stack like
> > `stl/bits/internal.h -> stl/bits/another.h -> stl/public_1.h ->
> > user_code.cc`, we say `public_1.h` exports `bits/internal.h` and add a
> > mapping from `bits/internal.h$` to `public_1.h`. But if we see another
> > (shorter) include stack like `stl/bits/internal.h -> stl/public_2.h ->
> > user_code.cc`, we say `stl/public_2.h` exports `stl/bits/internal.h`. This
> > heuristic works well for many cases. However, this may produce wrong
> > mappings when an internal header is used by multiple public headers. For
> > example, if we have two include stacks with the same length e.g.
> > `bits/internal.h -> public_1.h -> user.cc` and `bits/inernal.h ->
> > public_2.h -> user.cc`, the result mapping would depend on the order in
> > which we see these two stacks; thus, we had to do some manual adjustment to
> > make sure bits/internal.h is mapped to the correct header according to the
> > standard.
> >
> > I am happy to discuss better solution here. But within the scope of this
> > patch, I'd prefer to stick to interfaces that work well for the current
> > working solution instead of designing for potential future solutions. I
> > should be easy to iterate on the interfaces as these interfaces aren't
> > going to be widely used in clangd after all. WDYT?
> > I think this would depend on how you define the scope of this class. .inc
> > handling is a subtle case that I'm a bit hesitated to build into the
> > interface here.
>
> Sure it's subtle, but it's clearly in the scope of determining what the
> canonical header is for a symbol, which is the job of this class. We wouldn't
> be building it into the interface - on the contrary, the *current* proposed
> interface codifies *not* handling .inc files.
>
> But you're right that we should check in something that handles most cases.
>
> My preference would be to drop `.inc` from this patch until we can
> incorporate it into the design, but I'm also OK with a FIXME to move it.
Okay, I removed `.inc` handling from this patch ;)
Comment at: unittests/clangd/HeadersTests.cpp:29
+ // absolute path.
+ std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
+assert(llvm::sys::path::is_relative(File) && "FileName should be
relative");
sammccall wrote:
> sammccall wrote:
> > This test would be clearer to me if you removed this helper and just did
> >
> > ```FS.Files["sub/bar.h"] = ...```
> >
> > in the test.
> >
> > Can we change `buildTestFS` in `TestFS.cpp` to call
> > `getVirtualTestFilePath` on relative paths to allow this?
> >
> > (I can do this as a followup if you like, but it seems like a trivial
> > change)
> I thought better of that change to TestFS, but did some renames in r325326.
>
> So this would be `FS.Files[testPath("sub/bar.h")) = ...` which still seems
> more transparent - up to you.
Thanks a lot!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
sammccall added inline comments.
Comment at: unittests/clangd/HeadersTests.cpp:29
+ // absolute path.
+ std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
+assert(llvm::sys::path::is_relative(File) && "FileName should be
relative");
sammccall wrote:
> This test would be clearer to me if you removed this helper and just did
>
> ```FS.Files["sub/bar.h"] = ...```
>
> in the test.
>
> Can we change `buildTestFS` in `TestFS.cpp` to call `getVirtualTestFilePath`
> on relative paths to allow this?
>
> (I can do this as a followup if you like, but it seems like a trivial change)
I thought better of that change to TestFS, but did some renames in r325326.
So this would be `FS.Files[testPath("sub/bar.h")) = ...` which still seems more
transparent - up to you.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG apart from the .inc handling (happy to chat more)
Comment at: clangd/index/CanonicalIncludes.h:52
+ /// a canonical header name.
+ llvm::StringRef mapHeader(llvm::StringRef Header) const;
+
ioeric wrote:
> sammccall wrote:
> > So I'm a bit concerned this is too narrow an interface, and we really want
> > to deal with SourceLocation here which would give us the include stack.
> >
> > Evidence #1: .inc handling really is the same job, but because this class
> > has a single-file interface, we have to push it into the caller.
> > Evidence #2: I was thinking about a more... principled way of determining
> > system headers. One option would be to have a whitelist of public headers,
> > and walk up the include stack to see if you hit one. (I'm not 100% sure
> > this would work, but still...) This isn't implementable with the current
> > interface.
> >
> > One compromise would be to pass in a stack or something.
> > Efficiency doesn't really matter because the caller can cache based on the
> > top element.
> > Evidence #1: .inc handling really is the same job, but because this class
> > has a single-file interface, we have to push it into the caller.
> I think this would depend on how you define the scope of this class. `.inc`
> handling is a subtle case that I'm a bit hesitated to build into the
> interface here.
>
> > Evidence #2:
> This is actually very similar to how the hardcoded mapping was generated. I
> had a tool that examined include stacks for a library (e.g. STL) and applied
> a similar heuristic - treat the last header in the include stack within the
> library boundary as the "exporting" public header for a leaf include header,
> if there is no other public header that has shorter distance to that include.
> For example, if we see a stack like `stl/bits/internal.h ->
> stl/bits/another.h -> stl/public_1.h -> user_code.cc`, we say `public_1.h`
> exports `bits/internal.h` and add a mapping from `bits/internal.h$` to
> `public_1.h`. But if we see another (shorter) include stack like
> `stl/bits/internal.h -> stl/public_2.h -> user_code.cc`, we say
> `stl/public_2.h` exports `stl/bits/internal.h`. This heuristic works well for
> many cases. However, this may produce wrong mappings when an internal header
> is used by multiple public headers. For example, if we have two include
> stacks with the same length e.g. `bits/internal.h -> public_1.h -> user.cc`
> and `bits/inernal.h -> public_2.h -> user.cc`, the result mapping would
> depend on the order in which we see these two stacks; thus, we had to do some
> manual adjustment to make sure bits/internal.h is mapped to the correct
> header according to the standard.
>
> I am happy to discuss better solution here. But within the scope of this
> patch, I'd prefer to stick to interfaces that work well for the current
> working solution instead of designing for potential future solutions. I
> should be easy to iterate on the interfaces as these interfaces aren't going
> to be widely used in clangd after all. WDYT?
> I think this would depend on how you define the scope of this class. .inc
> handling is a subtle case that I'm a bit hesitated to build into the
> interface here.
Sure it's subtle, but it's clearly in the scope of determining what the
canonical header is for a symbol, which is the job of this class. We wouldn't
be building it into the interface - on the contrary, the *current* proposed
interface codifies *not* handling .inc files.
But you're right that we should check in something that handles most cases.
My preference would be to drop `.inc` from this patch until we can incorporate
it into the design, but I'm also OK with a FIXME to move it.
Comment at: unittests/clangd/HeadersTests.cpp:29
+ // absolute path.
+ std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
+assert(llvm::sys::path::is_relative(File) && "FileName should be
relative");
This test would be clearer to me if you removed this helper and just did
```FS.Files["sub/bar.h"] = ...```
in the test.
Can we change `buildTestFS` in `TestFS.cpp` to call `getVirtualTestFilePath` on
relative paths to allow this?
(I can do this as a followup if you like, but it seems like a trivial change)
Comment at: unittests/clangd/SymbolCollectorTests.cpp:50
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
nit: this is only used once, as Not(HasIncludeHeader()). Just use
IncludeHeader("")?
The slight difference in detail handling doesn't seem to matter (I'm not even
sure if it's exactly the right assertion)
Comment at: unittests/clangd/SymbolCollectorTests.cpp:624
+
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 134394.
ioeric added a comment.
- Merged with origin/master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,12 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
+}
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(DeclRange, Offsets, "") {
return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
arg.CanonicalDeclaration.EndOffset == Offsets.second;
@@ -62,41 +68,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", "-include",
@@ -117,12 +144,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
TEST_F(SymbolCollectorTest, CollectSymbols) {
@@ -555,6 +584,65 @@
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 133895.
ioeric marked 15 inline comments as done.
ioeric added a comment.
- Addressed some review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,12 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
+}
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -60,41 +66,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", TestFileName};
@@ -120,12 +147,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
TEST_F(SymbolCollectorTest, CollectSymbols) {
@@ -521,6 +550,65 @@
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric added inline comments.
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation)
> > > > and add an example?
> > > >
> > > > It might be worth explicitly stating that the result is an #include
> > > > string (with quotes) - you just call it a "path" here.
> > > >
> > > > "shortest" makes sense as a first implementation, but we may want to
> > > > document something more like "best" - I know there are codebases we
> > > > care about where file-relative paths are banned. Also I suspect we
> > > > don't give "../../../../Foo.h" even if it's shortest :-)
> > > > "shortest" makes sense as a first implementation, but we may want to
> > > > document something more like "best" - I know there are codebases we
> > > > care about where file-relative paths are banned. Also I suspect we
> > > > don't give "../../../../Foo.h" even if it's shortest :-)
> > >
> > > I think this part wasn't addressed
> > I added a comment for this in the header. But I might be misunderstanding
> > you suggestion. Did you mean we need a better name for the function?
> I think the function name is fine as a shorthand, just that the comment is a
> bit overspecified and possibly inaccurate compared to e.g. "Determines the
> preferred way to #include a file, taking into account the search path.
> Usually this will prefer a shorter representation like 'Foo/Bar.h' over a
> longer one like 'Baz/include/Foo/Bar.h'".
>
> I don't think the `../../../Foo.h` case is worth explicitly calling out - I
> just meant it as an example of why we *don't* want an overly-specific
> contract here.
I see. Thanks!
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:99
+CollectorOpts.CollectIncludePath = true;
+auto Includes = llvm::make_unique();
+addStandardLibraryMapping(Includes.get());
sammccall wrote:
> Do we create one of these per TU or per thread? The former is "clean" but
> seems potentially wasteful (compiling all those system header regexes for
> each TU). The latter is "fast" but potentially non-hermetic (can't think of a
> triggering case though).
>
> Maybe we should have a split between transient mappings (IWYU) and permanent
> ones?
This is created per TU now. In an earlier revision, this was one-per-program
because we statically constructed a regex map and passed the map into the
`CanonicalIncludes` via the constructor, like we do in include-fixer
(https://github.com/llvm-mirror/clang-tools-extra/blob/master/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp#L15).
But with the current design, I didn't do this because one-per-TU approach
seems to be cleaner, and the regex construction time seems to be relatively
small comparing to the time spent on actually compiling a TU.
If we really want to squeeze performance here, we would probably need either an
interface that takes a static regex mapping or one that takes pre-computed
`llvm::Regex`. But I'm not really sure if it would be worth it since this is
not a performance critical code path.
Comment at: clangd/index/CanonicalIncludes.cpp:74
+ {"include/__stddef_max_align_t.h$", ""},
+ {"include/__wmmintrin_aes.h$", ""},
+ {"include/__wmmintrin_pclmul.h$", ""},
sammccall wrote:
> these aren't standard library - deserves a comment?
>
> in general it looks like there's a bunch of standard library stuff, also
> posix stuff, and some compiler intrinsics.
> If this is the case, maybe "system headers" is a better descriptor than
> "standard library".
>
> Can we document approximately which standard libraries, which compiler
> extensions, and other standards (posix, but I guess windows one day) are
> included?
Switched both function and variable to "system headers" and updated the
documentation.
Comment at: clangd/index/CanonicalIncludes.h:10
+//
+// This file defines functionalities for remapping #include header for an index
+// symbol. For example, we can collect a mapping accoring to IWYU pragma
sammccall wrote:
> Can we split out the main ideas a bit? I think these are: a) what include
> mapping is, b) IWYU pragmas, c) standard library.
> We should also probably call out the relationship with the stuff in
> clangd/Headers.h.
>
> e.g.
> ```At indexing time, we decide which file to #included for a symbol.
> Usually this is the file with the canonical decl, but there are exceptions:
>
> - private headers may have pragmas pointing to the matching public header.
> (These are "IWYU" pragmas, named after the include-what-you-use tool).
> - the standard library is implemented in many files, without any
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
sammccall added a comment.
Insertion still LG (couple of nits, inline).
For indexing, my biggest questions:
- I worry CanonicalIncludes doesn't get enough information to make good
decisions - passing the include stack in some form may be better
- CanonicalIncludes has slightly weird patterns of reads/writes - most writes
are permanent and a few are transient, and it's not totally obvious how it
works in a multithreading context (though your code is correct). I'm not sure
whether this is worth fixing.
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
ioeric wrote:
> sammccall wrote:
> > sammccall wrote:
> > > maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation)
> > > and add an example?
> > >
> > > It might be worth explicitly stating that the result is an #include
> > > string (with quotes) - you just call it a "path" here.
> > >
> > > "shortest" makes sense as a first implementation, but we may want to
> > > document something more like "best" - I know there are codebases we care
> > > about where file-relative paths are banned. Also I suspect we don't give
> > > "../../../../Foo.h" even if it's shortest :-)
> > > "shortest" makes sense as a first implementation, but we may want to
> > > document something more like "best" - I know there are codebases we care
> > > about where file-relative paths are banned. Also I suspect we don't give
> > > "../../../../Foo.h" even if it's shortest :-)
> >
> > I think this part wasn't addressed
> I added a comment for this in the header. But I might be misunderstanding you
> suggestion. Did you mean we need a better name for the function?
I think the function name is fine as a shorthand, just that the comment is a
bit overspecified and possibly inaccurate compared to e.g. "Determines the
preferred way to #include a file, taking into account the search path. Usually
this will prefer a shorter representation like 'Foo/Bar.h' over a longer one
like 'Baz/include/Foo/Bar.h'".
I don't think the `../../../Foo.h` case is worth explicitly calling out - I
just meant it as an example of why we *don't* want an overly-specific contract
here.
Comment at: clangd/CodeComplete.cpp:290
+// We only insert #include for items with details, since we can't tell
+// weather the file URI of the canonical declaration would be the
+// canonical #include without checking IncludeHeader in the detail.
whether
Comment at: clangd/CodeComplete.cpp:301
+ Command Cmd;
+ // Command title is not added since this is not a user-facting
+ // command.
user-facing. unwrap?
Comment at: clangd/clients/clangd-vscode/package.json:1
{
"name": "vscode-clangd",
phab says you have ws-only changes in this file, which you might want to revert
Comment at: clangd/global-symbol-builder/CMakeLists.txt:1
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../)
(and here - ws-only changes?)
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:99
+CollectorOpts.CollectIncludePath = true;
+auto Includes = llvm::make_unique();
+addStandardLibraryMapping(Includes.get());
Do we create one of these per TU or per thread? The former is "clean" but seems
potentially wasteful (compiling all those system header regexes for each TU).
The latter is "fast" but potentially non-hermetic (can't think of a triggering
case though).
Maybe we should have a split between transient mappings (IWYU) and permanent
ones?
Comment at: clangd/index/CanonicalIncludes.cpp:51
+ PP.getSourceManager(), PP.getLangOpts());
+ size_t Pos = Text.find(IWYUPragma);
+ if (Pos == StringRef::npos)
I think this is
if (!Text.consume_front(IWYUPragma))
return false;
Comment at: clangd/index/CanonicalIncludes.cpp:72
+ static const std::vector
+ STLPostfixHeaderMap = {
+ {"include/__stddef_max_align_t.h$", ""},
"STL" :-)
Comment at: clangd/index/CanonicalIncludes.cpp:74
+ {"include/__stddef_max_align_t.h$", ""},
+ {"include/__wmmintrin_aes.h$", ""},
+ {"include/__wmmintrin_pclmul.h$", ""},
these aren't standard library - deserves a comment?
in general it looks like there's a bunch of standard library stuff, also posix
stuff, and some compiler intrinsics.
If this is the case, maybe "system headers" is a better descriptor than
"standard library".
Can we document approximately which standard libraries, which compiler
extensions, and other standards (posix, but I guess windows one day) are
included?
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric added inline comments. Comment at: clangd/ClangdServer.cpp:368 +/// Calculates the shortest possible include path when inserting \p Header to \p +/// File, by matching \p Header against all include search directories for \p sammccall wrote: > ioeric wrote: > > sammccall wrote: > > > This has a fair amount of logic (well, plumbing :-D) and uses relatively > > > little from ClangdServer. > > > Can we move `shortenIncludePath` to a separate file and pass in the FS > > > and command? > > > > > > I'd suggest maybe `Headers.h` - The formatting part of `insertInclude` > > > could also fit there, as well as probably the logic from > > > `switchSourceHeader` I think (the latter not in this patch of course). > > > > > > This won't really help much with testing, I just find it gets hard to > > > navigate files that have lots of details of unrelated features. > > > ClangdUnit and ClangdServer both have this potential to grow without > > > bound, though they're in reasonable shape now. Interested what you think! > > Done. > > > > I didn't move the formatting code, as half of the code is pulling out the > > style, which we might want to share/change depending on other clangd logics > > that might use style. > I'd still think pulling out `Expected > insertInclude(File, Code, Header, VFS, Style)` would be worthwhile here - the > formatting isn't a lot of code, but it's a bit magic, plus the quote > handling... it's a bit of code. It'd make it more obvious what the > interactions with ClangdServer's state are. But up to you, we can always do > this later. So it's just 2 lines for the replacement magic (+1 for comment) now after removing some redundant code. I like `shortenIncludePath` better because it's more self-contained and easier to write tests against, and `insertInclude` doesn't seem to carry much more weight while we would need to handle `replacements` logic which has been tested in the tests. Comment at: clangd/ClangdServer.cpp:370 +/// File, by matching \p Header against all include search directories for \p +/// File via `clang::HeaderSearch`. +/// sammccall wrote: > sammccall wrote: > > maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and > > add an example? > > > > It might be worth explicitly stating that the result is an #include string > > (with quotes) - you just call it a "path" here. > > > > "shortest" makes sense as a first implementation, but we may want to > > document something more like "best" - I know there are codebases we care > > about where file-relative paths are banned. Also I suspect we don't give > > "../../../../Foo.h" even if it's shortest :-) > > "shortest" makes sense as a first implementation, but we may want to > > document something more like "best" - I know there are codebases we care > > about where file-relative paths are banned. Also I suspect we don't give > > "../../../../Foo.h" even if it's shortest :-) > > I think this part wasn't addressed I added a comment for this in the header. But I might be misunderstanding you suggestion. Did you mean we need a better name for the function? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 133635.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/clients/clangd-vscode/package.json
clangd/global-symbol-builder/CMakeLists.txt
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,12 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
+}
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -60,41 +66,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", TestFileName};
@@ -120,12 +147,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
sammccall added a comment.
Insertion side LGTM, feel free to split and land.
Sorry I need to take off and will need to get to indexing on monday :(
Comment at: clangd/ClangdServer.cpp:368
+/// Calculates the shortest possible include path when inserting \p Header to
\p
+/// File, by matching \p Header against all include search directories for \p
ioeric wrote:
> sammccall wrote:
> > This has a fair amount of logic (well, plumbing :-D) and uses relatively
> > little from ClangdServer.
> > Can we move `shortenIncludePath` to a separate file and pass in the FS and
> > command?
> >
> > I'd suggest maybe `Headers.h` - The formatting part of `insertInclude`
> > could also fit there, as well as probably the logic from
> > `switchSourceHeader` I think (the latter not in this patch of course).
> >
> > This won't really help much with testing, I just find it gets hard to
> > navigate files that have lots of details of unrelated features. ClangdUnit
> > and ClangdServer both have this potential to grow without bound, though
> > they're in reasonable shape now. Interested what you think!
> Done.
>
> I didn't move the formatting code, as half of the code is pulling out the
> style, which we might want to share/change depending on other clangd logics
> that might use style.
I'd still think pulling out `Expected insertInclude(File,
Code, Header, VFS, Style)` would be worthwhile here - the formatting isn't a
lot of code, but it's a bit magic, plus the quote handling... it's a bit of
code. It'd make it more obvious what the interactions with ClangdServer's state
are. But up to you, we can always do this later.
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
sammccall wrote:
> maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and
> add an example?
>
> It might be worth explicitly stating that the result is an #include string
> (with quotes) - you just call it a "path" here.
>
> "shortest" makes sense as a first implementation, but we may want to document
> something more like "best" - I know there are codebases we care about where
> file-relative paths are banned. Also I suspect we don't give
> "../../../../Foo.h" even if it's shortest :-)
> "shortest" makes sense as a first implementation, but we may want to document
> something more like "best" - I know there are codebases we care about where
> file-relative paths are banned. Also I suspect we don't give
> "../../../../Foo.h" even if it's shortest :-)
I think this part wasn't addressed
Comment at: clangd/Headers.cpp:74
+
+ // We don't need to provide the content of \p File, as we are only interested
+ // in the include search directories in the compile command.
comment is outdated now
Comment at: clangd/Headers.cpp:114
+
+ log("Suggested #include is: " + Suggested);
+ return Suggested;
can you include the Header in this log message? (and possibly File, but that
might add more noise than signal)
Comment at: clangd/Headers.h:30
+/// \return A quoted "path" or . If \p Header is already (directly)
+/// included in the file (including those included via different paths), an
+/// error will be returned.
header-already-included is not an error condition.
Suggest returning llvm::Expected, or returning "" for this
case.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 133599.
ioeric added a comment.
- fix a leftover bug
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/clients/clangd-vscode/package.json
clangd/global-symbol-builder/CMakeLists.txt
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,12 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
+}
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -60,41 +66,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", TestFileName};
@@ -120,12 +147,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
TEST_F(SymbolCollectorTest, CollectSymbols) {
@@
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric added a comment.
Thanks! PTAL
Comment at: clangd/ClangdServer.cpp:465
+auto = Clang->getPreprocessor().getHeaderSearchInfo();
+std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+*Resolved, CompileCommand.Directory);
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > do we handle the case that the suggestion is already included?
> > > (including the case where it's included by a different name)
> > Yes and no. The current implementation only does textual matching against
> > existing #includes in the current file and inserts the header if no same
> > header was found. This complies with IWYU. But we are not handling the case
> > where the same header is included by different names. I added a `FIXME` for
> > this.
> I can't see the FIXME? (There's one in the header, but it doesn't seem to
> really cover this case)
>
> So this doesn't seem so hard: we can pass the file content, turn off
> recursive PP, and add PP callbacks to capture the names of each included file
> (the include-scanner patch does this).
> I'm not sure it's worth deferring, at least we should fix it soon before we
> lose context.
>
> But up to you, I'd suggest putting the fixme where we expect to fix it.
Since you prefer, I have included the change in the patch. I wanted to get to
this as soon as this patch is landed.
Comment at: clangd/ClangdServer.cpp:368
+/// Calculates the shortest possible include path when inserting \p Header to
\p
+/// File, by matching \p Header against all include search directories for \p
sammccall wrote:
> This has a fair amount of logic (well, plumbing :-D) and uses relatively
> little from ClangdServer.
> Can we move `shortenIncludePath` to a separate file and pass in the FS and
> command?
>
> I'd suggest maybe `Headers.h` - The formatting part of `insertInclude` could
> also fit there, as well as probably the logic from `switchSourceHeader` I
> think (the latter not in this patch of course).
>
> This won't really help much with testing, I just find it gets hard to
> navigate files that have lots of details of unrelated features. ClangdUnit
> and ClangdServer both have this potential to grow without bound, though
> they're in reasonable shape now. Interested what you think!
Done.
I didn't move the formatting code, as half of the code is pulling out the
style, which we might want to share/change depending on other clangd logics
that might use style.
Comment at: unittests/clangd/ClangdTests.cpp:849
+TEST_F(ClangdVFSTest, InsertIncludes) {
+ MockFSProvider FS;
sammccall wrote:
> Similarly, it'd be nice to pull these tests out into a test file parallel to
> the header.
> (As with the other tests, often it's easiest to actually test through the
> ClangdServer interface - this is mostly just for navigation)
Done. I left a simple test for replacements.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
ioeric updated this revision to Diff 133595.
ioeric marked 5 inline comments as done.
ioeric added a comment.
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/clients/clangd-vscode/package.json
clangd/global-symbol-builder/CMakeLists.txt
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/insert-include.test
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -47,6 +47,12 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
+}
+MATCHER_P(IncludeHeader, P, "") {
+ return arg.Detail && arg.Detail->IncludeHeader == P;
+}
MATCHER_P(LocationOffsets, Offsets, "") {
// Offset range in SymbolLocation is [start, end] while in Clangd is [start,
// end).
@@ -60,41 +66,62 @@
namespace {
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
- SymbolIndexActionFactory(SymbolCollector::Options COpts)
- : COpts(std::move(COpts)) {}
+ SymbolIndexActionFactory(SymbolCollector::Options COpts,
+ CommentHandler *PragmaHandler)
+ : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
clang::FrontendAction *create() override {
+class WrappedIndexAction : public WrapperFrontendAction {
+public:
+ WrappedIndexAction(std::shared_ptr C,
+ const index::IndexingOptions ,
+ CommentHandler *PragmaHandler)
+ : WrapperFrontendAction(
+index::createIndexingAction(C, Opts, nullptr)),
+PragmaHandler(PragmaHandler) {}
+
+ std::unique_ptr
+ CreateASTConsumer(CompilerInstance , StringRef InFile) override {
+if (PragmaHandler)
+ CI.getPreprocessor().addCommentHandler(PragmaHandler);
+return WrapperFrontendAction::CreateASTConsumer(CI, InFile);
+ }
+
+private:
+ index::IndexingOptions IndexOpts;
+ CommentHandler *PragmaHandler;
+};
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
IndexOpts.IndexFunctionLocals = false;
Collector = std::make_shared(COpts);
-FrontendAction *Action =
-index::createIndexingAction(Collector, IndexOpts, nullptr).release();
-return Action;
+return new WrappedIndexAction(Collector, std::move(IndexOpts),
+ PragmaHandler);
}
std::shared_ptr Collector;
SymbolCollector::Options COpts;
+ CommentHandler *PragmaHandler;
};
class SymbolCollectorTest : public ::testing::Test {
public:
SymbolCollectorTest()
- : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+ : InMemoryFileSystem(new vfs::InMemoryFileSystem),
+TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
TestHeaderURI = URI::createFile(TestHeaderName).toString();
TestFileURI = URI::createFile(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
const std::vector = {}) {
-llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-new vfs::InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-auto Factory = llvm::make_unique(CollectorOpts);
+auto Factory = llvm::make_unique(
+CollectorOpts, PragmaHandler.get());
std::vector Args = {"symbol_collector", "-fsyntax-only",
"-std=c++11", TestFileName};
@@ -120,12 +147,14 @@
}
protected:
+ llvm::IntrusiveRefCntPtr InMemoryFileSystem;
std::string TestHeaderName;
std::string TestHeaderURI;
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
SymbolCollector::Options CollectorOpts;
+ std::unique_ptr PragmaHandler;
};
[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.
sammccall added a comment.
Some comments on the insert side, which looks pretty good. I'll take another
look at indexing tomorrow.
Comment at: clangd/ClangdServer.cpp:465
+auto = Clang->getPreprocessor().getHeaderSearchInfo();
+std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+*Resolved, CompileCommand.Directory);
ioeric wrote:
> sammccall wrote:
> > do we handle the case that the suggestion is already included?
> > (including the case where it's included by a different name)
> Yes and no. The current implementation only does textual matching against
> existing #includes in the current file and inserts the header if no same
> header was found. This complies with IWYU. But we are not handling the case
> where the same header is included by different names. I added a `FIXME` for
> this.
I can't see the FIXME? (There's one in the header, but it doesn't seem to
really cover this case)
So this doesn't seem so hard: we can pass the file content, turn off recursive
PP, and add PP callbacks to capture the names of each included file (the
include-scanner patch does this).
I'm not sure it's worth deferring, at least we should fix it soon before we
lose context.
But up to you, I'd suggest putting the fixme where we expect to fix it.
Comment at: clangd/ClangdServer.cpp:368
+/// Calculates the shortest possible include path when inserting \p Header to
\p
+/// File, by matching \p Header against all include search directories for \p
This has a fair amount of logic (well, plumbing :-D) and uses relatively little
from ClangdServer.
Can we move `shortenIncludePath` to a separate file and pass in the FS and
command?
I'd suggest maybe `Headers.h` - The formatting part of `insertInclude` could
also fit there, as well as probably the logic from `switchSourceHeader` I think
(the latter not in this patch of course).
This won't really help much with testing, I just find it gets hard to navigate
files that have lots of details of unrelated features. ClangdUnit and
ClangdServer both have this potential to grow without bound, though they're in
reasonable shape now. Interested what you think!
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and add
an example?
It might be worth explicitly stating that the result is an #include string
(with quotes) - you just call it a "path" here.
"shortest" makes sense as a first implementation, but we may want to document
something more like "best" - I know there are codebases we care about where
file-relative paths are banned. Also I suspect we don't give
"../../../../Foo.h" even if it's shortest :-)
Comment at: clangd/ClangdServer.cpp:379
+ llvm::StringRef Header) {
+ // In order to get a HeaderSearch instance, we first create a
CompilerInstance
+ // and initialize it to get a Preprocessor which provides HeaderSearch.
This comment helps a lot. The subtext is: HeaderSearch is hard to construct
directly, so we're doing this weird dance.
I think this is worth calling out even louder - when I read this sort of code I
tend to take a *long* time to work out why the code seems to be doing unrelated
work.
Comment at: clangd/ClangdServer.cpp:420
+ auto = Clang->getPreprocessor().getHeaderSearchInfo();
+ std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+ Header, CompileCommand.Directory);
consume the optional `IsSystem` and use it to quote appropriately?
Comment at: unittests/clangd/ClangdTests.cpp:849
+TEST_F(ClangdVFSTest, InsertIncludes) {
+ MockFSProvider FS;
Similarly, it'd be nice to pull these tests out into a test file parallel to
the header.
(As with the other tests, often it's easiest to actually test through the
ClangdServer interface - this is mostly just for navigation)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
