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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 7 inline comments as done.
jkorous added a comment.

Hi Sam.

In https://reviews.llvm.org/D54799#1305470, @sammccall wrote:

>




>> I'd like to ask for early feedback - what's still missing is relevant client capability.
> 
> I'd suggest leaving it out unless others feel strongly.

More than happy to keep it simple! I somehow assumed there'll be demand for this capability.

>> - conditional return in `getCursorInfo` - Should we return for example data with empty `USR`?
> 
> Please return a symbol unless it has no SymbolID (we don't treat those as symbols in clangd).

Ok.

>> - `containerName` of local variables - It's currently empty even if semantic parent has a name (say it's a function). (Please search for local.cpp in the test.) Is that what we want?
> 
> good question, no idea.

I am adding an attempt to get a named declaration context for such cases. It's easy to remove if we decide against that.

>> - For now I used `getSymbolAtPosition()` as it's simpler and fits the context better. However I assume I could use this optimization from `tooling::getNamedDeclAt()` (in a separate patch): https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/#L82
> 
> Yes, though at least for C++ this optimization won't usually buy anything, as the top-level decl will just be a namespace decl I think.
>  We could plumb this deeper if we want to win more here.

Ok, I'll leave it for the hypothetical future.

> - One thing I am wondering about is whether we could use (and whether it's a significant improvement) some early return in `indexTopLevelDecls` (using `DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it correct to assume that at a given `Location` there can be maximum of one declaration and one definition? There can be multiple and we should return them all. (Not sure what we do for hover, but defs handles this correctly).

Ah, my bad - I was considering only explicit cases. Thanks for the explanation. In the context of textDocument/cursorInfo - does it make sense to use the first explicit declaration we find or am I missing something still? (This is related to discussion about `StopOnFirstDeclFound`.)

I processed and implemented most of your comments. Two major tasks are: finishing the discussion about multiple declarations and writing unittests (preferably after we agree on interfaces).



================
Comment at: Protocol.h:676
 
+/// Represents information about identifier.
+struct IdentifierData {
----------------
sammccall wrote:
> sammccall wrote:
> > Unless I'm misunderstanding, this is about a symbol, not an identifier. (e.g. overloads of C++ functions are distinct symbols). Maybe `SymbolDetails` would be a useful name.
> > 
> > There's a question of scope here: `getCursorInfo` seems to return a single instance of these, but there can be 0, 1, or many symbols at a given location. These could be `vector<SymbolDetails>` or so, or if this struct is intended to encapsulate them all, it could be named `SymbolsAtLocation` or so.
> /// This is returned from textDocument/cursorInfo, which is a clangd extension.
I probably don't understand "many symbols at a given location" - more at the end of my out-of-line comment.


================
Comment at: Protocol.h:678
+struct IdentifierData {
+  /// The name of this symbol.
+  std::string name;
----------------
sammccall wrote:
> This comment doesn't really say anything. It can be omitted, or we could add additional information.
> (Some of the existing comments are equally content-free, because we've copied the comments from LSP verbatim, but that's not the case here)
I was actually wondering what is the value of those comments but decided to keep it consistent. Deleting.


================
Comment at: Protocol.h:682
+  /// The name of the symbol containing this symbol.
+  std::string containerName;
+
----------------
sammccall wrote:
> sammccall wrote:
> > If we're not going to merge, I'd suggest making this "scope" and allowing it to end with ::.
> > This makes it easier to compose (qname = scope + name), and makes it clearer what "contains" means. (LSP got this wrong IMO, at least for C++).
> what do you want this to do for objc methods and fields?
> It'd be worth having a test if it's important, I'm guilty of not knowing/checking ObjC behavior.
I don't really have any opinion about this but @benlangmuir might have.

I'd just make sure whatever we decide makes sense for local symbols (if we keep returning them).

I'll add tests for ObjC. Thanks for pointing that out!


================
Comment at: Protocol.h:684
+
+  /// The USR of this symbol.
+  llvm::SmallString<128> USR;
----------------
sammccall wrote:
> USR could use some definition/references.
True. Would you go further than expanding the acronym and copying description from doxygen annotation of `SymbolID`?
I haven't actually found any better description in clang repository.

Shall I note `include/clang/Index/USRGeneration.h`?


================
Comment at: Protocol.h:689
+  /// Size is dependent on clang::clangd::SymbolID::RawValue.
+  llvm::SmallString<32> ID;
+};
----------------
sammccall wrote:
> can this just be SymbolID?
I didn't want to pull `SymbolID` from `index` originally but sure.

Are you fine with it living in `Protocol.h` or shall I move it/factor it out?


================
Comment at: Protocol.h:690
+  llvm::SmallString<32> ID;
+};
+llvm::json::Value toJSON(const IdentifierData &);
----------------
sammccall wrote:
> I think SymbolKind would be useful (in particular distinguishing macros might be important?)
> 
> This does point towards maybe adding to SymbolInformation rather than a new struct, but I'm not sure which is best.
AFAIK for our use-case the information in USR is good enough (e. g. "c:foo.cpp at 24@macro at FOO") so we might wait until we have a real need. WDYT?


================
Comment at: XRefs.cpp:95
   Preprocessor &PP;
+  const bool StopOnFirstDeclFound;
 
----------------
sammccall wrote:
> Please don't do this - it's inconsistent with the other XRefs features.
> (If for some reason you *really* want just one result, then this is easy to do on the client side if we get the API right)
Sure, let's discuss. I probably got confused by your suggestion of using `getSymbolAtPosition()` in https://reviews.llvm.org/D54529#1299531.

Do I understand it correctly that you mean visiting all declarations and selecting the single explicit one in `getSymbolInfo()`?
Or do you actually mean change of interface such as this? `llvm::Optional<SymbolDetails> getSymbolInfo()` -> `std::vector<SymbolDetails> getSymbolInfo()`.
Is my assumption that there could be only single explicit declaration for any given position correct in the first place?


================
Comment at: XRefs.cpp:770
+
+  if (!Symbols.Decls.empty()) {
+    if (const NamedDecl *ND = dyn_cast<NamedDecl>(Symbols.Decls.front().D)) {
----------------
sammccall wrote:
> (as alluded to elsewhere, we should return all decls/macros, not just one)
I replied to the above comment as it's related.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54799