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

Simon Marchi via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Feb 8 13:05:06 PST 2018


simark added a comment.

In https://reviews.llvm.org/D35894#1001875, @ilya-biryukov wrote:

> Thanks for picking this up!
>  I have just one major comment related to how we generate the hover text.
>
> Current implementation tries to get the actual source code for every declaration and then patch it up to look nice on in the output.
>  It is quite a large piece of code (spanning most of the patch) and tries to mix and match source code and output from the AST when there's not enough information in the AST (e.g. `tryFixupIndentation` and `stripOpeningBracket`). This approach is really complex as we'll inevitably try to parse "parts" of C++ and figure out how to deal with macros, etc.
>  Worse than that, I would expect this code to grow uncontrollably with time and be very hard to maintain.
>  Moreover, in presence of macros it arguably gives non-useful results. For example, consider this:
>
>   #define DEFINITIONS int foo(); int bar(); int baz();
>  
>   // somewhere else
>   DEFINITIONS
>  
>   // somewhere else
>   int test() {
>     foo(); // <-- hover currently doesn't work here. And even if it did, showing a line with just DEFINITIONS is not that useful.
>   }
>
>
> I suggest we move to a different approach of pretty-printing the relevant parts of the AST. It is already implemented in clang, handles all the cases in the AST (and will be updated along when AST is changed), shows useful information in presence of macros and is much easier to implement.
>  The version of `getHoverContents` using this is just a few lines of code:
>
>   static std::string getHoverContents(const Decl *D) {
>     if (TemplateDecl *TD = D->getDescribedTemplate())
>       D = TD; // we want to see the template bits.
>  
>     std::string Result;
>     llvm::raw_string_ostream OS(Result);
>  
>     PrintingPolicy Policy(D->getASTContext().getLangOpts());
>     Policy.TerseOutput = true;
>     D->print(OS, Policy);
>  
>     OS.flush();
>     return Result;
>   }
>
>
> It doesn't add the `"Defined in ..."` piece, but illustrates the APIs we can use. We should use the same APIs for the scope as well, avoiding fallbacks to manual printing if we can.
>  If there's something missing/wrong in the upstream pretty printers, it's fine and we can fix them (e.g., the thing that I've immediately noticed is that namespaces are always printed with curly braces).


I thought it would be nice to show in the hover the declaration formatted as it is written in the source file, because that's how the developer is used to read it.  But on the other hand, I completely agree that trying to do string formatting ourselves is opening a can of worms.  I'll try the approach you suggest.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894





More information about the cfe-commits mailing list