[PATCH] D34269: [clangd] Add "Go to Declaration" functionality

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.



================
Comment at: clangd/ClangdUnit.cpp:276
+class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::unique_ptr<std::vector<Location>> DeclarationLocations;
+  const SourceLocation &SearchedLocation;
----------------
Why do we need to use `unique_ptr<vector<Location>>` here and in other places instead of `vector<Location>`? 


================
Comment at: clangd/DeclarationLocationsFinder.cpp:48
+  // This can happen when nodes in the AST are visited twice.
+  if (std::none_of(DeclarationLocations.begin(), DeclarationLocations.end(),
+      [&L](const Location& Loc) {
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > 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?
> > Maybe use std::find instead of std::none_of?
> > ```
> > std::find(DeclarationLocations.begin(), DeclarationLocations.end(), L) == DeclarationLocations.end()
> > ```
> 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.
Totally agree, nice and simple.


================
Comment at: clangd/DeclarationLocationsFinder.cpp:59
+  Token Result;
+  if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(),
+      Unit.getASTContext().getLangOpts(), false)) {
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > 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?
> 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.
Awesome! 
clangIndex indeed never calls `handleMacroOccurence`, I wonder why it's there in the first place.


================
Comment at: test/clangd/definitions.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
----------------
malaperle-ericsson wrote:
> ilya-biryukov wrote:
> > 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.
> 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!
We could also reuse most of the code for all caret-based action, so it's actually useful for other things as well.


https://reviews.llvm.org/D34269