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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.



================
Comment at: clangd/ClangdServer.cpp:275
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  Units.runOnUnitWithoutReparse(
+      File, *FileContents.Draft, CDB, PCHs, TaggedFS.Value, [&](ClangdUnit &Unit) {
----------------
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).


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


================
Comment at: clangd/ClangdUnit.cpp:306
+        Pos.character);
+    SourceLocation PeekBeforeLocationEnd = Lexer::getLocForEndOfToken(
+        PeekBeforeLocation, 0, SourceMgr, Unit->getASTContext().getLangOpts());
----------------
Just wondering: is there a way to not do the lexing multiple times?


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


================
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:
> 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()
```


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


================
Comment at: clangd/DeclarationLocationsFinder.h:24
+/// Finds declarations locations that a given source location refers to.
+class DeclarationLocationsFinder : public index::IndexDataConsumer {
+  std::vector<Location> DeclarationLocations;
----------------
Maybe put this into ClangdUnit.cpp inside an anonymous namespace?
As it's just an implementation detail.


================
Comment at: clangd/DeclarationLocationsFinder.h:34
+
+  std::vector<Location> getLocations() {
+    return DeclarationLocations;
----------------
Maybe have a takeLocations() method that 'std::move's the vector instead of copying?


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


================
Comment at: clangd/Protocol.h:42
+
+  friend bool operator==(const URI &LHS, const URI &RHS) {
+    return LHS.uri == RHS.uri;
----------------
Maybe add 'operator != ' for consistency?


================
Comment at: clangd/Protocol.h:98
+
+  friend bool operator==(const Location &LHS, const Location &RHS) {
+    return LHS.uri == RHS.uri && LHS.range == RHS.range;
----------------
Maybe add 'operator != ' for consistency?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D34269