[PATCH] D48159: [clangd] Implement hover for "auto" and "decltype"

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is so great, thank you!



================
Comment at: clangd/XRefs.cpp:544
+/// Computes the deduced type at a given location by visiting the relevant
+/// nodes.
+class DeducedTypeVisitor : public RecursiveASTVisitor<DeducedTypeVisitor> {
----------------
maybe a bit more context: "we use this to display the actual type when hovering over an `auto` keyword or `decltype()` expression"


================
Comment at: clangd/XRefs.cpp:546
+class DeducedTypeVisitor : public RecursiveASTVisitor<DeducedTypeVisitor> {
+  const SourceLocation &SearchedLocation;
+  llvm::Optional<QualType> DeducedType;
----------------
nit: SourceLocation is 32 bits, just copy it?
(Reference seems fine here but less obvious than it could be)


================
Comment at: clangd/XRefs.cpp:559
+  //- auto& i = 1;
+  bool VisitDeclaratorDecl(DeclaratorDecl *D) {
+    if (!D->getTypeSourceInfo() ||
----------------
out of curiosity, why not implement `VisitTypeLoc` and handle all the cases where it turns out to be `auto` etc?
Even for `auto&` I'd expect the inner `auto` to have a `TypeLoc` you could visit, saving the trouble of unwrapping.

(I'm probably wrong about all this, I don't know the AST well. But I'd like to learn!)


================
Comment at: clangd/XRefs.cpp:640
+/// Retrieves the deduced type at a given location (auto, decltype).
+llvm::Optional<QualType> getDeducedType(ParsedAST &AST,
+                                        SourceLocation SourceLocationBeg) {
----------------
This is a really nice standalone piece of logic.
It might be slightly cleaner to put it in `AST.h` with fine-grained unit-tests there, and just smoke test it here. That way this file is more focused on *what* the features do, and the lower layer has details about how they work.
(And if we find another use for this, like a "replace with deduced type" refactoring, we could easily reuse it).

That said, the current stuff in this file doesn't exhibit that layering/separation today. Happy if you prefer to land it here, and I/someone may do such a refactoring in the future.


================
Comment at: clangd/XRefs.cpp:656
+  DeducedTypeVisitor V(SourceLocationBeg);
+  V.TraverseTranslationUnitDecl(ASTCtx.getTranslationUnitDecl());
+  return V.getDeducedType();
----------------
This will end up deserializing the whole preamble I think, which is slow.
The fix is just to traverse from each of the top-level *non-preamble* decls instead, i.e. `AST.getLocalTopLevelDecls()`.

(we've fixed this bug many times so far, it would be nice if there's a systematic way to fix/catch this but I can't think of one)


================
Comment at: unittests/clangd/TestTU.h:47
 
-  ParsedAST build() const;
+  ParsedAST build(const std::vector<const char *> &ExtraArgs = {}) const;
   SymbolSlab headerSymbols() const;
----------------
headerSymbols() and index() also depend on these flags - can you make these extra args a member instead? (can be public, just like Code)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48159