sammccall added a comment. comments on interface again, will take another pass at implementation
================ Comment at: clang-tools-extra/clangd/XRefs.h:65 + /// Fully qualiffied name for the scope containing the Sym. + std::string Scope; + std::string ParentScope; ---------------- kadircet wrote: > sammccall wrote: > > what's the difference between Scope and ParentScope? > > Can we give them more obvious names, or a comment? > > (The current comment doesn't really say anything) > We've been using Name and Scope to represent two parts of qualified names > across the codebase. > Comment was unfortunately misplaced :/ LMK if you have any other choices for > the name Yes, but here you've got three concepts: name/scope/parentscope (now called containedin?). And this covers lots of cases that our existing Name/Scope doesn't really (there are lots of symbols we don't index). It wasn't a rhetorical question, I can't suggest better names because I don't know what they are. My suggestion would be to split into namespace scope/local scope/name, such that Name has no `::`, `NamespaceScope` has exactly the (non-inline) enclosing namespaces, and `NamespaceScope + LocalScope + Name == QName`. So for a local in a class member, `NamespaceScope == "clang::clangd""`, `LocalScope == "SymbolCollector::consume::` and `Name = Symbols`. Or just `Name = Symbols` and the others are blank, depending on how we choose to display locals. ================ Comment at: clang-tools-extra/clangd/XRefs.h:53 +/// Contains detailed information about a Symbol. Especially useful when +/// generating hover responses. It is not serialized so that embedding clients +/// can choose to serialize it w.r.t their UI capabilities. ---------------- I'm having trouble following the "serialized" sentence. Maybe "It can be rendered as a hover panel, or embedding clients can use the structured information to provide their own UI"? ================ Comment at: clang-tools-extra/clangd/XRefs.h:76 + /// - None for deduced types, e.g "auto", "decltype" keywords. + llvm::Optional<std::string> ContainedIn; + SymbolKind Kind; ---------------- This seems overlapping/non-orthogonal with scope. It doesn't let us render a member as `ClassName::method` for instance. If we really need "In function x::Y" rather than "in x::Y", we could add a member "ImmediateScopeKind" or something. But I'm not sure we do. ================ Comment at: clang-tools-extra/clangd/XRefs.h:89 + llvm::Optional<std::vector<Param>> Parameters; + /// Set for all template declarations(function, class, variable). + llvm::Optional<std::vector<Param>> TemplateParameters; ---------------- nit: just "templates"? (e.g. if we hover over a call to std::swap, we might be showing the instantiation rather than the declaration?) ================ Comment at: clang-tools-extra/clangd/XRefs.h:93 + /// Lower to LSP struct. + Hover render() const; +}; ---------------- Even if this struct ends up containing the range, it might be more obvious to have render() produce the `MarkupContent` only, leaving the caller to pull out the range themselves. Converting the range isn't closely related to rendering, and I think for cleanest layering/testing you want to return `FormattedString` after rL360151 (which doesn't have a `Hover` equivalent). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61497/new/ https://reviews.llvm.org/D61497 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
