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

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.


================
Comment at: clangd/ClangdServer.h:306
+  /// Run formatting for \p Rng inside \p File.
+  std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
+  /// Run formatting for the whole \p File.
----------------
ilya-biryukov wrote:
> These functions are duplicates of existing ones, they should not be here.
> Bad merge?
Yes, this is a bad merge.


================
Comment at: clangd/ClangdUnit.cpp:438
+  // trimming the extra "::"
+  std::string Res;
+  PrintingPolicy WithScopePP(LangOpts);
----------------
ilya-biryukov wrote:
> We should avoid doing the string-matching when we have the AST.
> Use something like this instead (haven't tested, but you should get the idea):
> 
> ```
> /// Returns a NamedDecl that could be used to print the qualifier.
> Decl* getScopeOfDecl(Decl* D) {
>   // Code from NamedDecl::printQualifiedName, we should probably move it into 
>   const DeclContext *Ctx = D->getDeclContext();
> 
>   // For ObjC methods, look through categories and use the interface as context.
>   if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
>     if (auto *ID = MD->getClassInterface())
>       Ctx = ID;
> 
>   if (Ctx->isFunctionOrMethod()) {
>     /// This is a function-local entity.
>     return nullptr;
>   }
> 
>   return Ctx;
> }
> 
> 
> std::string printScopeOfDecl(Decl* Scope) {
>   if (isa<TranslationUnitDecl>(Scope))
>     return "global namespace";
> 
>   if (isa<NamedDecl>(Scope))
>     return cast<NamedDecl>(Scope)->printQualifiedName();
> 
>   return "";
> }
> 
> 
> // Later use like this:
> auto ScopeText = printScopeOfDecl(getScopeOfDecl(D));
> ```
> 
I'm not sure I understand. The intention was to have a separate MarkedString display the scope for whatever we are hovering on. The way I understand the above code, this would display a scope that isn't as precise as I would like which defeats the purpose of having this feature in the first place.


================
Comment at: clangd/ClangdUnit.cpp:498
   Position Begin;
-  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+  Begin.line = SourceMgr.getExpansionLineNumber(LocStart) - 1;
+  Begin.character = SourceMgr.getExpansionColumnNumber(LocStart) - 1;
----------------
ilya-biryukov wrote:
> Why are we changing the semantics here? Why should we use expansion locations instead of spelling locations here?
Bad merge.


================
Comment at: clangd/ClangdUnit.cpp:591
+// one line at a time to determine what will be included.
+SourceLocation getFunctionComments(ParsedAST &AST, const Decl *D) {
+
----------------
ilya-biryukov wrote:
> This code seems pretty involved. Don't `RawComment`s handle this case?
Bad merge.


================
Comment at: clangd/ClangdUnit.cpp:968
       // createInvocationFromCommandLine sets DisableFree.
+      CI->LangOpts->CommentOpts.ParseAllComments = true;
       CI->getFrontendOpts().DisableFree = false;
----------------
ilya-biryukov wrote:
> This should clearly go before the comment!
> Also, we're currently setting this flag when building the preamble, but not when parsing the AST. We should definitely do that in both cases.
Do you mean also adding this line somewhere in ASTUnit.cpp?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894