[PATCH] D54799: [clangd][WIP] textDocument/SymbolInfo method

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG from my side.
If you have unit tests in the next couple of days, happy to take a look, otherwise go ahead once Alex/Ben are happy.

(I'm at work today/tomorrow and then I'm going to be away through new year's, so don't wait for me)



================
Comment at: clangd/ClangdServer.cpp:529
+
+  WorkScheduler.runWithAST("CursorInfo", File, Bind(Action, std::move(CB)));
+}
----------------
sammccall wrote:
> nit: SymbolInfo
(This still says CursorInfo in the runWithAST call)


================
Comment at: clangd/ClangdServer.h:204
 
+  /// Get symbol info for given position.
+  void symbolInfo(PathRef File, Position Pos,
----------------
Maybe mention "clangd extension" here.
We should really set up some more structured docs for these one day, but a comment is something.


================
Comment at: clangd/Protocol.cpp:429
+  return json::Object{
+      {"name", P.name},
+      {"containerName", P.containerName},
----------------
sammccall wrote:
> for each of the attributes that can be logically absent, we should probably serialize it conditionally (or serialize it as null).
> 
> We're usually happy enough to use sentinel values like "" to mean missing internally, but we try not to send them over the wire.
> (SymbolInformation isn't a great example unfortunately...)
elsewhere we tend to write:

```
json::Object Result{
 ... mandatory fields ...
};
```
if (!P.name.empty())
  Result["name"] = P.name;
```

etc. I find this a little bit more readable than the ternary expressions (due to the need for conversions) but up to you.


================
Comment at: clangd/XRefs.cpp:785
+    }
+    Results.emplace_back(std::move(NewSymbol));
+  }
----------------
jkorous wrote:
> sammccall wrote:
> > nit: push_back (and below)
> Sure, no problem. I have to admit I thought these two are the same but I guess you pointed this out because there's a functional difference. I'd appreciate if you could advise where can I learn the details.
Here the behavior is identical, so this is really a style thing.

`emplace_back` is a more powerful function, with semantics "make a new element": it will forward *any* argument list to a constructor of Symbol, selected by overload resolution.

`push_back` is a more constrained function, with semantics "insert this element": it takes one Symbol and forwards it to the move constructor (or copy constructor, as appropriate).

They're equivalent here because `emplace_back` can select the move-constructor when passed a single `Symbol&&`. But it communicates intent less clearly, so humans have a harder time (harder to understand the code) as does the compiler (error messages are worse).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54799/new/

https://reviews.llvm.org/D54799