[PATCH] D38639: [clangd] #include statements support for Open definition

This revision was automatically updated to reflect the committed changes. Closed by commit rL325662: [clangd] #include statements support for Open definition (authored by malaperle, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 134549. malaperle added a comment. Change some EXPECT_TRUE to ASSERT_TRUE Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h clangd/SourceCode.cpp

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM modulo the `ASSERT_TRUE` nit. Comment at: unittests/clangd/XRefsTests.cpp:295 + runFindDefinitions(Server, FooCpp, SourceAnnotations.point()); +

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 134432. malaperle added a comment. Use EXPECT_THAT in a few places and clean-ups in tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added a comment. Last drop of NITs. After they're fixed, this change is ready to land! Comment at: unittests/clangd/XRefsTests.cpp:282 + const char *HeaderContents = R"cpp([[]]int a;)cpp"; + Annotations HeaderAnnoations(HeaderContents); + FS.Files[FooH] =

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 134296. malaperle added a comment. Fix some NITs, add more tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h clangd/SourceCode.cpp clangd/SourceCode.h

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: unittests/clangd/XRefsTests.cpp:53 +class IgnoreDiagnostics : public DiagnosticsConsumer { + void onDiagnosticsReady( malaperle wrote: > ilya-biryukov wrote: > > NIT: remove this class, use `IgnoreDiagnostics`

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added inline comments. Comment at: unittests/clangd/XRefsTests.cpp:245 + const char *SourceContents = R"cpp( + #include "$2^invalid.h" + #include "^foo.h" ilya-biryukov wrote: > Could we also add tests for corner cases: cursor before opening quote,

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added inline comments. Comment at: unittests/clangd/XRefsTests.cpp:53 +class IgnoreDiagnostics : public DiagnosticsConsumer { + void onDiagnosticsReady( ilya-biryukov wrote: > NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h`

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:103 + +if (SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) { + // Only inclusion directives in the main file make sense. The user cannot NIT: replace

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added inline comments. Comment at: clangd/ClangdUnit.h:51 +using IncludeReferenceMap = std::unordered_map; + ilya-biryukov wrote: > We use `unordered_map` as a `vector>` here. (i.e. we never look up > values by key,

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 133978. malaperle added a comment. Herald added subscribers: ioeric, jkorous-apple. Move tests to XRefsTests, change IncludeReferenceMap to a vector and rename it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files:

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.h:51 +using IncludeReferenceMap = std::unordered_map; + We use `unordered_map` as a `vector>` here. (i.e. we never look up values by key, because we don't only

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 129293. malaperle added a comment. Revert an unintentional white space change. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h clangd/SourceCode.cpp

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle updated this revision to Diff 129292. malaperle added a comment. Store map in PremableData and copy it on reparse. Convert SourceLoc to Position when comparing with include Positions. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added inline comments. Comment at: clangd/ClangdUnit.cpp:113 + CppFilePreambleCallbacks(IncludeReferenceMap ) + : IRM(IRM) {} ilya-biryukov wrote: > malaperle wrote: > > ilya-biryukov wrote: > > > ilya-biryukov wrote: > > > > Let's create a new

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added a comment. @malaperle, hi! Both new diff and updating this works, so feel free the one that suits you best. I tend to look over the whole change on each new round of reviews anyway. Comment at: clangd/ClangdUnit.cpp:113 +

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added a comment. @ilya-biryukov Hi! I'll be updating William's patches that were in progress. I just have a few comments/question before I send a new update. (I also don't know if I can update this diff or I have to create a new diff on Phabricator... I guess we'll see!!).

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 127900. Nebiroth marked 11 inline comments as done. Nebiroth added a comment. Minor code cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:183 + unsigned CharNumber = SourceMgr.getSpellingColumnNumber( + DeclMacrosFinder->getSearchedLocation()); + ilya-biryukov wrote: > Replace with

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Another round of review Comment at: clangd/ClangdUnit.cpp:113 + CppFilePreambleCallbacks(IncludeReferenceMap ) + : IRM(IRM) {}

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 127617. Nebiroth added a comment. Removed some useless inclusions Removed superfluous check when inserting data in map Moved addition to DeclarationLocations in finish() outside of DeclMacrosFinder Merged with revision 321087 (moved findDefinitions

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth marked 8 inline comments as done. Nebiroth added inline comments. Comment at: clangd/ClangdUnit.cpp:85 + +if (SourceMgr.getMainFileID() == SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) && SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin()))

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Thanks for addressing the comments quickly. I took another look and added a few more comments. This moves in the right direction, though, this is really close to

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 127386. Nebiroth marked 14 inline comments as done. Nebiroth added a comment. Herald added a subscriber: mgrang. CppFilePreambleCallbacks no longer extends PPCallbacks CppFilePreambleCallbacks no longer needs SourceManager parameter Removed temporary

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov requested changes to this revision. ilya-biryukov added inline comments. This revision now requires changes to proceed. Comment at: clangd/ClangdServer.cpp:446 std::vector Result; - Resources->getAST().get()->runUnderLock([Pos, , this](ParsedAST *AST) { +

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 127161. Nebiroth marked 27 inline comments as done. Nebiroth added a comment. inner class IncludeReferenceMap replaced by one map fillRangeVector() and findPreambleIncludes() code moved into wrapper class Open definition now returns an empty Range struct

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle requested changes to this revision. malaperle added inline comments. This revision now requires changes to proceed. Comment at: clangd/ClangdServer.cpp:454 + +IncludeReferenceMap IRM = std::move(AST->takeIRM()); +Result = clangd::findDefinitions(*AST, Pos,

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 126429. Nebiroth added a comment. Creating unique_ptr for wrapper class now uses overriden method createPPCallbacks() from PrecompiledPreamble class Moved wrapper class to inline inside createPPCallbacks() Wrapper class now uses

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:38 +class DelegatingPPCallbacks : public PPCallbacks { + Nebiroth wrote: > ilya-biryukov wrote: > > What's the purpose of this class? > We need to be able to use a wrapper class to be able

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 125814. Nebiroth added a comment. Fixed re-added include Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38639 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Protocol.h

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 125813. Nebiroth added a comment. Herald added a subscriber: klimek. Using PPCallbacks interface to find non-preamble includes Created inner class to store vectors in to find locations Refactored methods to remove some unnecessary parameters Refactored Unit

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth marked an inline comment as done. Nebiroth added inline comments. Comment at: clangd/ClangdUnit.cpp:38 +class DelegatingPPCallbacks : public PPCallbacks { + ilya-biryukov wrote: > What's the purpose of this class? We need to be able to use a wrapper

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added a comment. In https://reviews.llvm.org/D38639#920487, @ilya-biryukov wrote: > I think we should never iterate through `SourceManager`, as it's much easier > to get wrong than using the callbacks. I agree that all that fiddling with > callbacks is unfortunate, but it's well

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:368 std::vector Result; - Resources->getAST().get()->runUnderLock([Pos, , this](ParsedAST *AST) { -if (!AST) - return; -Result = clangd::findDefinitions(*AST, Pos, Logger); - }); +

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added a comment. In https://reviews.llvm.org/D38639#920427, @malaperle wrote: > I'm not sure it's better to use the InclusionDirective callback for this. We > need to get the includes in two places: in the preamble and non-preamble. In > the preamble we use the callback, have to

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle added a comment. I'm not sure it's better to use the InclusionDirective callback for this. We need to get the includes in two places: in the preamble and non-preamble. In the preamble we use the callback, have to store some temporary stuff because we don't have a SourceManager in

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 120485. Nebiroth added a comment. - Fixed adding incorrect test file. https://reviews.llvm.org/D38639 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/GlobalCompilationDatabase.cpp clangd/Protocol.h

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 120482. Nebiroth added a comment. - Now overriding InclusionDirective as a callback to get proper includes information. - Fixed tests. https://reviews.llvm.org/D38639 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:103 + void AfterExecute(CompilerInstance ) override { +const SourceManager = CI.getSourceManager(); Nebiroth wrote: > ilya-biryukov wrote: > > There's a much better public API to get

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth marked 21 inline comments as done. Nebiroth added inline comments. Comment at: clangd/ClangdUnit.cpp:103 + void AfterExecute(CompilerInstance ) override { +const SourceManager = CI.getSourceManager(); ilya-biryukov wrote: > There's a much better

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. Looking forward to getting this change! I miss this as well. Please take a look at my comments, though. I think we might want to use a different API to implement this. Comment at:

[PATCH] D38639: [clangd] #include statements support for Open definition

malaperle requested changes to this revision. malaperle added inline comments. This revision now requires changes to proceed. Comment at: clangd/ClangdUnit.cpp:81 + std::map takeIncludeMap() { +return std::move(IncludeMap);

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth updated this revision to Diff 118046. Nebiroth added a comment. Fixed accidental removal of CheckSourceHeaderSwitch test https://reviews.llvm.org/D38639 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h unittests/clangd/ClangdTests.cpp Index:

[PATCH] D38639: [clangd] #include statements support for Open definition

Nebiroth created this revision. ctrl-clicking on #include statements now opens the file being pointed by that statement. https://reviews.llvm.org/D38639 Files: clangd/ClangdServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h unittests/clangd/ClangdTests.cpp Index: