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:
https://reviews.llvm.org/D38639
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
clangd/Sourc
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());
+ EXPE
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
clangd/SourceCode.c
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] = Heade
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
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` f
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, c
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` instead.
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 `FilenameRange.get
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, because we don't only the range, m
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:
clangd/Clang
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 the range, merely a point in the
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
clangd/
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
malaperle added inline comments.
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > Let's create a
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
+ CppFilePreambleCallbacks(Includ
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!!).
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
clangd/XRefs
ilya-biryukov added inline comments.
Comment at: clangd/XRefs.cpp:183
+ unsigned CharNumber = SourceMgr.getSpellingColumnNumber(
+ DeclMacrosFinder->getSearchedLocation());
+
ilya-biryukov wrote:
> Replace with `DeclMacrosFinder->getSearchedLocation
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(IRM) {}
---
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 an
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())) {
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 landing.
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 ve
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, &Result, this](ParsedAST *AST)
{
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 (lin
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, Logge
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 CppFilePreambleCallback
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 t
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
unittests/clangd/ClangdTests
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 t
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 cl
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 worth
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:368
std::vector Result;
- Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST)
{
-if (!AST)
- return;
-Result = clangd::findDefinitions(*AST, Pos, Logger);
- });
+
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 s
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 Incl
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
unittests/clan
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
clangd/
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
Nebiroth wrote:
> ilya-biryukov wrote:
> > There's a much better public API to
Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+const SourceManager &SM = CI.getSourceManager();
ilya-biryukov wrote:
> There's a much b
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: clangd/ClangdSer
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);
takeIncludeLocationMap?
==
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: unittests/c
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: unittests/clangd/Cl
45 matches
Mail list logo
