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@24@macro@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
