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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle-ericsson added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:303
+  // fo|o -> foo|  good!
+  if (InputLocation == SourceLocationBeg && Pos.character > 0) {
+    SourceLocation PeekBeforeLocation = Unit->getLocation(FE, Pos.line + 1,
----------------
ilya-biryukov wrote:
> 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.
I like it better with the return


================
Comment at: clangd/ClangdUnit.cpp:306
+        Pos.character);
+    SourceLocation PeekBeforeLocationEnd = Lexer::getLocForEndOfToken(
+        PeekBeforeLocation, 0, SourceMgr, Unit->getASTContext().getLangOpts());
----------------
ilya-biryukov wrote:
> Just wondering: is there a way to not do the lexing multiple times?
I was able to simplify this a bit. There's on only one call to getRawToken and one to GetBeginningOfToken.


================
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) {
----------------
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.


================
Comment at: clangd/DeclarationLocationsFinder.cpp:59
+  Token Result;
+  if (!Lexer::getRawToken(SearchedLocation, Result, Unit.getSourceManager(),
+      Unit.getASTContext().getLangOpts(), false)) {
----------------
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.


================
Comment at: clangd/Protocol.cpp:57
 std::string URI::unparse(const URI &U) {
-  return U.uri;
+  return "\"" + U.uri + "\"";
 }
----------------
ilya-biryukov wrote:
> Why this didn't require any changes to other code? This method hasn't been used before?
No it wasn't used before.


================
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.
----------------
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!


https://reviews.llvm.org/D34269