Page MenuHomePhabricator

[clangd] #include statements support for Open definition
ClosedPublic

Authored by Nebiroth on Oct 6 2017, 11:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ilya-biryukov requested changes to this revision.Dec 19 2017, 1:39 AM

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.

clangd/ClangdServer.cpp
25 ↗(On Diff #127386)

Is this include redundant now? Can we remove it?

430 ↗(On Diff #127386)

Capturing this seems redundant. Remove it?

clangd/ClangdUnit.cpp
85 ↗(On Diff #127386)

clang-format please

85 ↗(On Diff #127386)

Do we need both checks? Doesn't SourceMgr.isInMainFile handles all the cases?

113 ↗(On Diff #127386)

Let's create a new empty map inside this class and have a takeIncludeReferences method (similar to TopLevelDeclIDs and takeTopLevelDeclIDs)

147 ↗(On Diff #127386)

We should have SourceMgr at all the proper places now, let's store IncludeReferenceMap directly

281 ↗(On Diff #127386)

What reference does this IncludeReferenceMap contain? Includes from the preamble?

347 ↗(On Diff #127386)

This class handles processing AST and preprocessor, it does not need to get IncludeLocationMap in constructor or store it at all.
Remove IncludeLocationMap from this class and handle getting references from IncludeLocationMap outside this class instead.

clangd/ClangdUnit.h
97 ↗(On Diff #127386)

Make it all const?

const IncludeReferenceMap &getIRM() const { return IRM; }

276 ↗(On Diff #127386)

This function moved without need and lost a comment.

This revision now requires changes to proceed.Dec 19 2017, 1:39 AM
Nebiroth marked 8 inline comments as done.Dec 19 2017, 2:15 PM
Nebiroth added inline comments.
clangd/ClangdUnit.cpp
85 ↗(On Diff #127386)

This check was here to prevent an issue that appeared previously. I don't think this check is needed anymore.

147 ↗(On Diff #127386)

The idea was to pass a single map around to fill with every reference instead of having separate maps being merged together.

281 ↗(On Diff #127386)

This should contain every reference available for one file.

Nebiroth marked 2 inline comments as done.Dec 19 2017, 3:38 PM
Nebiroth updated this revision to Diff 127617.Dec 19 2017, 3:55 PM
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 and findDocumentHighlights
ilya-biryukov requested changes to this revision.Dec 20 2017, 10:17 AM

Another round of review

clangd/ClangdUnit.cpp
141 ↗(On Diff #127617)

Maybe add assert(SourceMgr && "SourceMgr must be set at this point") here?

281 ↗(On Diff #127617)

Don't we already store the map we need in PreambleData? Why do we need an extra IRM parameter?

113 ↗(On Diff #127386)

This comment is not addressed yet, but marked as done.

147 ↗(On Diff #127386)

You copy the map for preamble and then append to it in CppFilePreambleCallbacks? That also LG, we should not have many references there anyway.

281 ↗(On Diff #127386)

Thanks for clarifying, LG.

clangd/ClangdUnit.h
276 ↗(On Diff #127386)

Now we have duplicated definitions here. Bad merge?

clangd/CodeComplete.cpp
328 ↗(On Diff #127617)

Accidental change?

807 ↗(On Diff #127617)

Accidental change?

clangd/XRefs.cpp
41 ↗(On Diff #127617)

We don't need locations anymore. Remove them.

177 ↗(On Diff #127617)

Probably a good place for a comment. Also, maybe rename local var to something easier to understand like IncludeDefinitions

/// Process targets for paths inside #include directive.
std::vector<Location> IncludeTargets;
178 ↗(On Diff #127617)

No need to special case empty maps, remove the if altogether.

183 ↗(On Diff #127617)

Replace with DeclMacrosFinder->getSearchedLocation() SourceLocationBeg, it makes the code easier to read.

185 ↗(On Diff #127617)

why do we need to convert to unsigned? To slience compiler warnings?

188 ↗(On Diff #127617)

this should be R.start.charater <= CharNumber && CharNumber <= R.end.character

This revision now requires changes to proceed.Dec 20 2017, 10:17 AM
ilya-biryukov added inline comments.Dec 21 2017, 7:39 AM
clangd/XRefs.cpp
183 ↗(On Diff #127617)

Sorry, the comment got messed up. Here's the correct version:

Replace DeclMacrosFinder->getSearchedLocation() with SourceLocationBeg, it makes the code easier to read.

Nebiroth updated this revision to Diff 127900.Dec 21 2017, 9:11 AM
Nebiroth marked 11 inline comments as done.
Minor code cleanup

@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!!).

clangd/ClangdUnit.cpp
281 ↗(On Diff #127617)

Since the map will be now stored in ParsedAST and the instance doesn't exist yet, we need to keep the IRM parameter. I noticed that it wasn't being std::move'd though.

113 ↗(On Diff #127386)

As mentioned below, the idea was to have a single map being appended to, without having to merge two separate maps. However, I can change the code so that two separate maps are built and merged if you prefer.

But I'm not so clear if that's what you'd prefer:

You copy the map for preamble and then append to it in CppFilePreambleCallbacks? That also LG, we should not have many references there anyway.

It's not meant to have any copy. The idea was to create a single IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble and non-preamble include references and std::move it around for later use (stored in ParsedAST).

clangd/XRefs.cpp
185 ↗(On Diff #127617)

Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber returns unsigned. I'll extract another var for the line number and cast both to int instead to have less casts and make the condition smaller.

@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.

clangd/ClangdUnit.cpp
281 ↗(On Diff #127617)

That looks fine. Also see the other comment on why we need to copy the map from the Preamble, and not std::move it.

113 ↗(On Diff #127386)

We can't have a single map because AST is rebuilt more often than the Preamble, so we have two options:

  • Store a map for the preamble separately, copy it when we need to rebuild the AST and append references from the AST to the new instance of the map.
  • Store two maps: one contains references only from the Preamble, the other one from the AST.

I think both are fine, since the copy of the map will be cheap anyway, as we only store a list of includes inside the main file.

clangd/XRefs.cpp
185 ↗(On Diff #127617)

Can we maybe convert to clangd::Position using the helper methods first and do the comparison of two clangd::Positions?
Comparing between clangd::Position and clang's line/column numbers is a common source of off-by-one errors in clangd.

malaperle added inline comments.Jan 10 2018, 9:36 AM
clangd/ClangdUnit.cpp
113 ↗(On Diff #127386)

We can't have a single map because AST is rebuilt more often than the Preamble, so we have two options:

Doh! Indeed.

OK so I added the map in PreambleData, this one will say every reparse unless the preamble changes. When the AST is built/rebuilt, the map is copied (or empty) and includes from the AST are appended to it then stored in ParsedAST (option #1?).

clangd/XRefs.cpp
185 ↗(On Diff #127617)

offsetToPosition (if that's what you mean) uses a StringRef of the code, which is not handy in this case. I'll add another one "sourceLocToPosition" to convert a SourceLocation to a Position. WDYT? It can be used in a few other places too.

malaperle updated this revision to Diff 129292.Jan 10 2018, 9:38 AM

Store map in PremableData and copy it on reparse.
Convert SourceLoc to Position when comparing with include Positions.

malaperle resigned from this revision.Jan 10 2018, 9:39 AM
malaperle updated this revision to Diff 129293.Jan 10 2018, 9:43 AM

Revert an unintentional white space change.

ilya-biryukov added inline comments.Jan 11 2018, 2:22 AM
clangd/ClangdUnit.h
51 ↗(On Diff #129293)

We use unordered_map as a vector<pair<>> here. (i.e. we never look up values by key, because we don't only the range, merely a point in the range)
Replace map with vector<pair<>> and remove RangeHash that we don't need anymore.

clangd/XRefs.cpp
185 ↗(On Diff #127617)

Looks good, thanks!

unittests/clangd/ClangdTests.cpp
781 ↗(On Diff #129293)

We moved findDefinition tests to XRefsTests.cpp and added a useful Annotation helper to make writing these tests simpler.
Maybe we could use it for this test as well?

malaperle updated this revision to Diff 133978.Feb 12 2018, 7:09 PM

Move tests to XRefsTests, change IncludeReferenceMap to a vector and rename it.

malaperle added inline comments.Feb 12 2018, 7:10 PM
clangd/ClangdUnit.h
51 ↗(On Diff #129293)

Done. I also renamed, IncludeReferenceMap to InclusionLocations. I thought I was more suitable.

unittests/clangd/ClangdTests.cpp
781 ↗(On Diff #129293)

Done. (I don't know why I can't use the "done" checkbox, maybe because I wasn't the original author?)

ilya-biryukov added inline comments.Feb 13 2018, 6:07 AM
clangd/ClangdUnit.cpp
103 ↗(On Diff #133978)

NIT: replace FilenameRange.getAsRange() with SR

126 ↗(On Diff #133978)

NIT: remove ctor, use initializer on the member instead:

private:
  SourceManager *SourceMgr = nullptr;
160 ↗(On Diff #133978)

Maybe swap IncLocations and SourceMgr? Grouping TopLevelDecls, TopLevelDeclIDs and IncLocations together seems reasonable, as all of them are essentially output parameters.

296 ↗(On Diff #133978)

NIT: move IncLocations closer to their first use, i.e. create the variable right before addPPCallbacks()

clangd/ClangdUnit.h
51 ↗(On Diff #129293)

LG

105 ↗(On Diff #133978)

NIT: move to .cpp file to be consisten with other decls in this file.

clangd/XRefs.cpp
171 ↗(On Diff #133978)

NIT: SourceMgr does not depend on the loop variable, declare it out of the loop.

174 ↗(On Diff #133978)

NIT: define operator <= for Position and do this instead: R.start <= Pos && Pos < R.end (note that LSP ranges are half-open, i.e. the end is excluded).
Even better: define a bool contains(Position Pos) const helper in the Range class and use it here (will abstract away to half-open nature of the range)

178 ↗(On Diff #133978)

Let's follow the pattern of decls and macros here. I.e. not return from the function, merely collect all the results.

unittests/clangd/XRefsTests.cpp
53 ↗(On Diff #133978)

NIT: remove this class, use IgnoreDiagnostics from Compiler.h instead.

245 ↗(On Diff #133978)

Could we also add tests for corner cases: cursor before opening quote, cursor after the closing quote, cursor in the middle of #include token? (we shouldn't navigate anywhere in the middle of the #include token)

malaperle added inline comments.Feb 13 2018, 10:05 AM
unittests/clangd/XRefsTests.cpp
53 ↗(On Diff #133978)

It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that wouldn't work.

malaperle added inline comments.Feb 13 2018, 9:42 PM
unittests/clangd/XRefsTests.cpp
245 ↗(On Diff #133978)

cursor before opening quote, cursor after the closing quote

I assume we don't want to navigate anywhere for these positions? I don't have an opinion.

(we shouldn't navigate anywhere in the middle of the #include token)

It did in CDT and I thought it worked nicely as it made it easier to click on it. You can hold ctrl on the whole line and it underlined it (well except trailing comments). But clients don't handle the spaces nicely (underline between #include and file name) so I thought I'd work on the client first before making the server do it. Anyhow, for now it shouldn't navigate indeed.

ilya-biryukov added inline comments.Feb 14 2018, 5:36 AM
unittests/clangd/XRefsTests.cpp
53 ↗(On Diff #133978)

You're right, I got confused, sorry. Disregard my comment.

245 ↗(On Diff #133978)

> cursor before opening quote, cursor after the closing quote
I assume we don't want to navigate anywhere for these positions? I don't have an opinion.

I'd say we should navigate when the cursor is right before the opening quote. For the closing quote I don't have any opinions either :-)

Fix some NITs, add more tests.

Last drop of NITs. After they're fixed, this change is ready to land!

unittests/clangd/XRefsTests.cpp
282 ↗(On Diff #134296)

There's a typo, should be HeaderAnnotations

302 ↗(On Diff #134296)

NIT: one assertion using EXPECT_THAT will read better and produce more helpful messages in case of failures:

// Create FooHUri to avoid typing URIForFile everywhere.
auto FooHUri = URIForFile{FooH.str()};

EXPECT_THAT(Locations, ElementsAre(Location{FooHUri, HeaderAnnoations.range()}));
328 ↗(On Diff #134296)

NIT: Use EXPECT_THAT here too:
EXPECT_THAT(Locations, IsEmpty());

malaperle updated this revision to Diff 134432.Feb 15 2018, 7:51 AM

Use EXPECT_THAT in a few places and clean-ups in tests.

ilya-biryukov accepted this revision.Feb 15 2018, 8:50 AM

LGTM modulo the ASSERT_TRUE nit.

unittests/clangd/XRefsTests.cpp
295 ↗(On Diff #134432)

Maybe replace with ASSERT_TRUE?
If Locations has an error, an assertion will fire in debug mode inside Expected that the error was not handled.

This revision is now accepted and ready to land.Feb 15 2018, 8:50 AM
malaperle updated this revision to Diff 134549.Feb 15 2018, 7:27 PM

Change some EXPECT_TRUE to ASSERT_TRUE

ilya-biryukov accepted this revision.Feb 16 2018, 2:02 AM
This revision was automatically updated to reflect the committed changes.