[PATCH] D35894: [clangd] Code hover for Clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

I copy/pasted the comments again so that you don't miss them.



================
Comment at: clangd/ClangdUnit.cpp:1117
+
+  if (!DeclVector.empty()) {
+    const Decl *LocationDecl = DeclVector[0];
----------------
malaperle wrote:
> malaperle wrote:
> > I think we need to simplify this part a bit. I'll try to find a way. Feel free to wait until more comments before updating.
> Here's the version in which I tried to simplify this *a bit*. With the new ErrorOr checks as well.
> 
> ```
> Hover clangd::getHover(ParsedAST &AST, Position Pos, clangd::Logger &Logger) {
>   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
>   const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
>   const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
>   if (!FE)
>     return Hover();
> 
>   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
> 
>   auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
>       llvm::errs(), SourceLocationBeg, AST.getASTContext(),
>       AST.getPreprocessor());
>   index::IndexingOptions IndexOpts;
>   IndexOpts.SystemSymbolFilter =
>       index::IndexingOptions::SystemSymbolFilterKind::All;
>   IndexOpts.IndexFunctionLocals = true;
>   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
>                      DeclMacrosFinder, IndexOpts);
> 
>   auto Decls = DeclMacrosFinder->takeDecls();
>   if (!Decls.empty() && Decls[0]) {
>     const Decl *LocationDecl = Decls[0];
>     std::vector<MarkedString> HoverContents;
> 
>     // Compute scope as the first "section" of the hover.
>     if (const NamedDecl *NamedD = dyn_cast<NamedDecl>(LocationDecl)) {
>       std::string Scope = getScope(NamedD, AST.getASTContext().getLangOpts());
>       if (!Scope.empty()) {
>         MarkedString Info = {"", "C++", "In " + Scope};
>         HoverContents.push_back(Info);
>       }
>     }
> 
>     // Adjust beginning of hover text depending on the presence of templates and comments.
>     TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
>     const Decl * BeginDecl = TDec ? TDec : LocationDecl;
>     SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
>     RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
>         BeginDecl);
>     if (RC)
>       BeginLocation = RC->getLocStart();
> 
>     // Adjust end of hover text for things that have braces/bodies. We don't
>     // want to show the whole definition of a function, class, etc.
>     SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
>     if (auto FuncDecl = dyn_cast<FunctionDecl>(LocationDecl)) {
>       if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
>         EndLocation = FuncDecl->getBody()->getLocStart();
>     } else if (auto TagDeclaration = dyn_cast<TagDecl>(LocationDecl)) {
>       if (TagDeclaration->isThisDeclarationADefinition())
>         EndLocation = TagDeclaration->getBraceRange().getBegin();
>     } else if (auto NameDecl = dyn_cast<NamespaceDecl>(LocationDecl)) {
>       SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
>           LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
>       if (BeforeLBraceLoc.isValid())
>         EndLocation = BeforeLBraceLoc;
>     }
> 
>     SourceRange SR(BeginLocation, EndLocation);
>     if (SR.isValid()) {
>       auto L = getDeclarationLocation(AST, SR);
>       if (L) {
>         auto Ref = getBufferDataFromSourceRange(AST, *L);
>         if (Ref) {
>           Hover H;
>           if (SourceMgr.isInMainFile(BeginLocation))
>             H.range = L->range;
>           MarkedString MS = {"", "C++", *Ref};
>           HoverContents.push_back(MS);
>           H.contents = std::move(HoverContents);
>           return H;
>         }
>       }
>     }
>   }
> 
>   auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
>   if (!MacroInfos.empty() && MacroInfos[0]) {
>     const MacroInfo *MacroInf = MacroInfos[0];
>     SourceRange SR(MacroInf->getDefinitionLoc(),
>                                  MacroInf->getDefinitionEndLoc());
>     if (SR.isValid()) {
>       auto L = getDeclarationLocation(AST, SR);
>       if (L) {
>         auto Ref = getBufferDataFromSourceRange(AST, *L);
>         if (Ref) {
>           Hover H;
>           if (SourceMgr.isInMainFile(SR.getBegin()))
>             H.range = L->range;
>           MarkedString MS = {"", "C++", "#define " + Ref->str()};
>           H.contents.push_back(MS);
>           return H;
>         }
>       }
>     }
>   }
> 
>   return Hover();
> }
> ```
Here's the version in which I tried to simplify this *a bit*. With the new ErrorOr checks as well.

```
Hover clangd::getHover(ParsedAST &AST, Position Pos, clangd::Logger &Logger) {
  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
  const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
  const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
  if (!FE)
    return Hover();

  SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);

  auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
      llvm::errs(), SourceLocationBeg, AST.getASTContext(),
      AST.getPreprocessor());
  index::IndexingOptions IndexOpts;
  IndexOpts.SystemSymbolFilter =
      index::IndexingOptions::SystemSymbolFilterKind::All;
  IndexOpts.IndexFunctionLocals = true;
  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
                     DeclMacrosFinder, IndexOpts);

  auto Decls = DeclMacrosFinder->takeDecls();
  if (!Decls.empty() && Decls[0]) {
    const Decl *LocationDecl = Decls[0];
    std::vector<MarkedString> HoverContents;

    // Compute scope as the first "section" of the hover.
    if (const NamedDecl *NamedD = dyn_cast<NamedDecl>(LocationDecl)) {
      std::string Scope = getScope(NamedD, AST.getASTContext().getLangOpts());
      if (!Scope.empty()) {
        MarkedString Info = {"", "C++", "In " + Scope};
        HoverContents.push_back(Info);
      }
    }

    // Adjust beginning of hover text depending on the presence of templates and comments.
    TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
    const Decl * BeginDecl = TDec ? TDec : LocationDecl;
    SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
    RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
        BeginDecl);
    if (RC)
      BeginLocation = RC->getLocStart();

    // Adjust end of hover text for things that have braces/bodies. We don't
    // want to show the whole definition of a function, class, etc.
    SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
    if (auto FuncDecl = dyn_cast<FunctionDecl>(LocationDecl)) {
      if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
        EndLocation = FuncDecl->getBody()->getLocStart();
    } else if (auto TagDeclaration = dyn_cast<TagDecl>(LocationDecl)) {
      if (TagDeclaration->isThisDeclarationADefinition())
        EndLocation = TagDeclaration->getBraceRange().getBegin();
    } else if (auto NameDecl = dyn_cast<NamespaceDecl>(LocationDecl)) {
      SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
          LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
      if (BeforeLBraceLoc.isValid())
        EndLocation = BeforeLBraceLoc;
    }

    SourceRange SR(BeginLocation, EndLocation);
    if (SR.isValid()) {
      auto L = getDeclarationLocation(AST, SR);
      if (L) {
        auto Ref = getBufferDataFromSourceRange(AST, *L);
        if (Ref) {
          Hover H;
          if (SourceMgr.isInMainFile(BeginLocation))
            H.range = L->range;
          MarkedString MS = {"", "C++", *Ref};
          HoverContents.push_back(MS);
          H.contents = std::move(HoverContents);
          return H;
        }
      }
    }
  }

  auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
  if (!MacroInfos.empty() && MacroInfos[0]) {
    const MacroInfo *MacroInf = MacroInfos[0];
    SourceRange SR(MacroInf->getDefinitionLoc(),
                                 MacroInf->getDefinitionEndLoc());
    if (SR.isValid()) {
      auto L = getDeclarationLocation(AST, SR);
      if (L) {
        auto Ref = getBufferDataFromSourceRange(AST, *L);
        if (Ref) {
          Hover H;
          if (SourceMgr.isInMainFile(SR.getBegin()))
            H.range = L->range;
          MarkedString MS = {"", "C++", "#define " + Ref->str()};
          H.contents.push_back(MS);
          return H;
        }
      }
    }
  }

  return Hover();
}
```


================
Comment at: clangd/ClangdUnit.cpp:1172
+        Location L = getDeclarationLocation(AST, SR);
+        H.range = L.range;
+        Ref = getDataBufferFromSourceRange(AST, SR, L);
----------------
malaperle wrote:
> The range could be in another file but we can only highlight things in the file that the user current has open. For example, if I'm foo.cpp and I hover on something declared in foo.h, it will change the background color in foo.cpp but using the line numbers of foo.h! The protocol should be more clear about this but since there are no Uri in the Hover struct, we can assume this range should only apply to the open file, i.e. the main file. So I suggest this check:
>           if (SourceMgr.isInMainFile(BeginLocation))
>             H.range = L->range;
> 
The range could be in another file but we can only highlight things in the file that the user current has open. For example, if I'm foo.cpp and I hover on something declared in foo.h, it will change the background color in foo.cpp but using the line numbers of foo.h! The protocol should be more clear about this but since there are no Uri in the Hover struct, we can assume this range should only apply to the open file, i.e. the main file. So I suggest this check:
```
if (SourceMgr.isInMainFile(BeginLocation))
  H.range = L->range;
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894