This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add "Go to Declaration" functionality
ClosedPublic

Authored by malaperle-ericsson on Jun 15 2017, 8:36 PM.

Details

Summary

This change allows to navigate to most identifiers' declarations in code. This is a first step towards implementing "Go to Definition". It reuses clangIndex in order to detect which occurrences corresponds to the position requested. The occurrences' Decls are then used to generate locations suitable for navigating to the declarations.

Event Timeline

malaperle-ericsson set the repository for this revision to rL LLVM.
malaperle-ericsson added a project: Restricted Project.
malaperle-ericsson added a subscriber: cfe-commits.
ilya-biryukov edited edge metadata.Jun 16 2017, 4:06 AM

Hi @malaperle-ericsson. Thanks for the patch.
IndexingAction has a very simple interface, exactly what clangd needs and nothing more.

BTW, we had a bug open for that: https://bugs.llvm.org/show_bug.cgi?id=33261, feel free to reassign to yourself.

clangd/ClangdServer.cpp
275

We must call runOnUnit here, calling runOnUnitWithoutReparse will result in an outdated AST.
(This can't happen if you use -run-synchronously, so tests don't catch it).

clangd/ClangdUnit.cpp
303

Minor: Maybe invert condition to reduce nesting, i.e. if (InputLocation != SourceLocationBeg || Pos.character == 0) return SourceLocationBeg;

PS. I'm perfectly fine if this comment is ignored, it's just a matter of preference.

306

Just wondering: is there a way to not do the lexing multiple times?

clangd/DeclarationLocationsFinder.cpp
48

Is it possible for DeclarationLocation to become large, so that quadratic behavior is observed?

If not, maybe add an assert that it's smaller than some threshold?
If yes, maybe use std::set instead of std::vector or use vector and later std::sort and std::unique in the end?

48

Maybe use std::find instead of std::none_of?

std::find(DeclarationLocations.begin(), DeclarationLocations.end(), L) == DeclarationLocations.end()
59

Could we implement handleMacroOccurence instead?
I suspect current code won't work if macro is undef'ed/redefined later:

#define FOO 123

int b = FO|O;

#define FOO 125
// or
// #undef FOO

Maybe also add this to tests?

clangd/DeclarationLocationsFinder.h
24

Maybe put this into ClangdUnit.cpp inside an anonymous namespace?
As it's just an implementation detail.

34

Maybe have a takeLocations() method that 'std::move's the vector instead of copying?

clangd/Protocol.cpp
57

Why this didn't require any changes to other code? This method hasn't been used before?

clangd/Protocol.h
42

Maybe add 'operator != ' for consistency?

98

Maybe add 'operator != ' for consistency?

test/clangd/definitions.test
1

Could we also add more readable tests specifically for that?
I propose to have a tool that could read files like:

int aaa;
int b = a/*{l1}*/aa;
int c = /*{l2}*/b;

And have it output the resulting goto results:

l1 -> {main.cpp:0:4} int |aaa;
l2 -> {main.cpp:1:4} int |b;

And we could also have a tool that prints expected clangd input/output based on that to check that action actually works.
It's not directly relevant to this patch, though. Just random thoughts of what we should do for easier testing.

malaperle-ericsson marked 12 inline comments as done.

Address comments

clangd/ClangdUnit.cpp
303

I like it better with the return

306

I was able to simplify this a bit. There's on only one call to getRawToken and one to GetBeginningOfToken.

clangd/DeclarationLocationsFinder.cpp
48

I went with std::sort/std::unique. I don't think this will ever be large but I don't think it would be good to have an arbitrary limit either. I think keeping the vector and cleaning it later is nice and simple.

59

You're right! It didn't work properly in this case. I added a few tests.
For handleMacroOccurence, it actually isn't even called so we'd have to improve the clangIndex code to do this. I think this is a good first step though.

clangd/Protocol.cpp
57

No it wasn't used before.

test/clangd/definitions.test
1

I think it's a good idea! Although I wonder if it's a bit too much work for something that very specific to "textDocument/definition". I fully agree that the tests need to be more readable and it would be worth a try!

Looks good now, but I think we definitely need to change unique_ptr<vector<Location>> to vector<Location> before submitting it.

I won't be available until next Wednesday, so feel free to submit without my final approval.

clangd/ClangdUnit.cpp
273

Why do we need to use unique_ptr<vector<Location>> here and in other places instead of vector<Location>?

clangd/DeclarationLocationsFinder.cpp
48

Totally agree, nice and simple.

59

Awesome!
clangIndex indeed never calls handleMacroOccurence, I wonder why it's there in the first place.

test/clangd/definitions.test
1

We could also reuse most of the code for all caret-based action, so it's actually useful for other things as well.

clangd/ClangdUnit.cpp
273

It was to implement "takeLocations". Calling it claims ownship of the vector. Did you have something else in mind when you suggested to implement takeLocations with a std::move? Disclaimer: this c++11 stuff is all new to me so I wouldn't be surprised if I'm doing it in a terrible way.

clangd/ClangdUnit.cpp
273

I guess I can make takeLocations return a vector&& and the other places will return a normal vector RVO will take care of not making copies?

ilya-biryukov added inline comments.Jun 28 2017, 1:46 AM
clangd/ClangdUnit.cpp
273

Just return std::vector and std::move the original field. It will have the same behaviour (i.e. no copies of the vector data, ownership transferred to the caller) without extra heap allocs:

  std::vector<Location> takeLocations() {
    // .... Do postprocessing here, i.e. sort+unique+erase
    return std::move(DeclarationLocations);
  }
// ...
private:
  std::vector<Location> DeclarationLocations;

Remove use of unique_ptr

This revision is now accepted and ready to land.Jun 28 2017, 7:36 AM