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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:941
           createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine, VFS);
+      CI->LangOpts->CommentOpts.ParseAllComments = true;
       // createInvocationFromCommandLine sets DisableFree.
----------------
Doesn't this slow down parsing by a lot? I thought @ilya-biryukov had seen that it was responsible for big performance losses in code complete.

ISTM we might prefer to look up the comment from the index by USR, and just live without it if we can't find it.

(Apologies if this has been discussed)



================
Comment at: clangd/ClangdUnit.h:1
 //===--- ClangdUnit.h -------------------------------------------*- C++-*-===//
 //
----------------
Please don't cram more stuff into clangdunit that's not directly related to building ASTs and managing their lifetimes. Instead, can we pull as much as possible out into `Documentation.h` or similar?

Note not `Hover.h` as there are other use cases, like we should use this logic for the code completion item documentation too.


================
Comment at: test/clangd/hover.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
----------------
Could I ask you to do primary/exhaustive testing of this with unit tests, and just a basic smoke test with lit?
Then it's possible to read the tests, understand whether they're correct, and understand the failure messages.

I'm trying to pull more of the features in this direction - see `CodeCompleteTests.cpp` for an example of how this can work.

We've had quite a few cases of bugs that had tests, but nobody noticed that the tests were wrong because it's just not possible to read them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894