sammccall added a comment.
(I only got about halfway through the implementation - it's missing a lot of
comments I think ;-))
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:539
-/// Generate a \p Hover object given the declaration \p D.
-static Hover getHoverContents(const Decl *D) {
- Hover H;
- llvm::Optional<std::string> NamedScope = getScopeName(D);
+static QualType getDeclType(const Decl *D) {
+ if (const TypedefNameDecl *TDD = dyn_cast<TypedefNameDecl>(D))
----------------
I think this is basically ASTContext::getTypeDeclType()?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:549
- // Generate the "Declared in" section.
- if (NamedScope) {
- assert(!NamedScope->empty());
+static llvm::Optional<Location> getDeclLoc(const SourceLocation &SL,
+ ASTContext &AST) {
----------------
what does this have to do with decls?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:563
- H.contents.value += "Declared in ";
- H.contents.value += *NamedScope;
- H.contents.value += "\n\n";
+static std::string getDefinitionLine(const Decl *D) {
+ std::string Definition;
----------------
printDefinition?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:574
- // We want to include the template in the Hover.
- if (TemplateDecl *TD = D->getDescribedTemplate())
- D = TD;
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+ const std::vector<HoverInfo::Param> &Params) {
----------------
I don't think it's reasonable to define this private helper as an overload of
operator<<.
Make it a function, or inline it? (I think used only once)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1207
+ const HoverInfo::Param &P) {
+ OS << P.T << ' ' << P.Name;
+ if (!P.Default.empty())
----------------
avoid emitting the space if T/name are empty?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1214
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const HoverInfo &HI) {
+ OS << HI.Definition << '\n';
+ OS << HI.Documentation << '\n';
----------------
This doesn't seem sufficiently self-documenting.
================
Comment at: clang-tools-extra/clangd/XRefs.h:52
+struct HoverInfo {
+ LocatedSymbol Sym;
+ /// Name of the context containing the symbol.
----------------
kadircet wrote:
> sammccall wrote:
> > I'm not sure about reuse of LocatedSymbol - do we want to commit to
> > returning decl/def locations?
> > Name might be enough here.
> It might be nice to provide editors with enough info to jump to definition(it
> was brought up during last meeting). But happy to reduce it to just name.
(this is not done I think - LocatedSymbol is still here)
================
Comment at: clang-tools-extra/clangd/XRefs.h:73
+ llvm::Optional<std::vector<Param>> Parameters;
+ llvm::Optional<std::vector<Param>> TemplateParameters;
+
----------------
ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > What does `Type` mean for non-type and template template parameters?
> > > Could you add a comment?
> > added comment for template template parameters.
> >
> > For non-type template params, isn't it clear that this will hold the type
> > of that param? e.g `template <C<int>... X>` -> `C<int>...`
> Ah, sure, sorry, I meant the type parameters :-)
I get the curiosity, but this is too much text, and only covers template
parameters which are not the most important case.
Can we just say something like `The pretty-printed parameter type, e.g. "int",
or "typename" (in TemplateParameters)`?
We should do something sensible for template template parameters, but it's not
important or non-obvious enough to document.
================
Comment at: clang-tools-extra/clangd/XRefs.h:52
+struct HoverInfo {
+ using Type = std::string;
----------------
This needs docs :-)
================
Comment at: clang-tools-extra/clangd/XRefs.h:54
+ using Type = std::string;
+ struct Param {
+ // For template template params it holds the whole template decl, e.g,
----------------
one line doc for this struct?
================
Comment at: clang-tools-extra/clangd/XRefs.h:58
+ // For type-template params will contain "typename" or "class".
+ Type T;
+ std::string Name;
----------------
needs a real name
mention the no-type case (macros)
================
Comment at: clang-tools-extra/clangd/XRefs.h:59
+ Type T;
+ std::string Name;
+ std::string Default;
----------------
mention the unnamed case
================
Comment at: clang-tools-extra/clangd/XRefs.h:60
+ std::string Name;
+ std::string Default;
+ };
----------------
mention the no-default case
================
Comment at: clang-tools-extra/clangd/XRefs.h:64
+ LocatedSymbol Sym;
+ /// Fully qualiffied name for the scope containing the Sym.
+ std::string Scope;
----------------
qualiffied -> qualified
================
Comment at: clang-tools-extra/clangd/XRefs.h:65
+ /// Fully qualiffied name for the scope containing the Sym.
+ std::string Scope;
+ std::string ParentScope;
----------------
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)
================
Comment at: clang-tools-extra/clangd/XRefs.h:69
+ std::string Documentation;
+ /// Line containing the definition of the symbol.
+ std::string Definition;
----------------
This sounds like it's a source location, (e.g. "file.cc:42:7")
But I think it's rather source code?
================
Comment at: clang-tools-extra/clangd/XRefs.h:72
+
+ /// T and ReturnType are mutually exclusive.
+ llvm::Optional<Type> T;
----------------
I'm not sure this is significant or should even be true (e.g. if T is a
function reference?)
Could we explain the relationship between the different fields here instead,
maybe briefly with examples? (A function will have return type and parameters
set, etc)
================
Comment at: clang-tools-extra/clangd/XRefs.h:73
+ /// T and ReturnType are mutually exclusive.
+ llvm::Optional<Type> T;
+ llvm::Optional<Type> ReturnType;
----------------
T needs a real name.
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
