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

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

malaperle marked 4 inline comments as done.
malaperle added inline comments.


================
Comment at: clangd/XRefs.cpp:559
+  //- auto& i = 1;
+  bool VisitDeclaratorDecl(DeclaratorDecl *D) {
+    if (!D->getTypeSourceInfo() ||
----------------
sammccall wrote:
> 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!)
>From what I saw, there are actually two different AutoType* for each textual "auto". The AutoType* containing the deduced type does not get visited via a typeloc. It's not entirely clear to me why since I don't know the AST well either. I was thinking maybe the first is created when the type is not deduced yet and later on, then the rest of the function or expression is parsed, a second one with the actual type deduced is created. If I look at the code paths where they are created, it seems like this is roughly what's happening. The first one is created when the declarator is parsed (no deduced type yet) and the second is created when the expression of the initializer (or return statement) is evaluated and the type is then deduced. The visitor only visits the first one's typeloc. I don't think I'm knowledgeable enough to say whether or not that's a bug but it seems on purpose that it is modelled this way. Although it would be much nicer to only have to visit typelocs...


================
Comment at: clangd/XRefs.cpp:640
+/// Retrieves the deduced type at a given location (auto, decltype).
+llvm::Optional<QualType> getDeducedType(ParsedAST &AST,
+                                        SourceLocation SourceLocationBeg) {
----------------
sammccall wrote:
> 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.
Good idea! The refactoring would be a neat feature too. I think I'd prefer to leave this refactoring for a bit later since this patch looks like it's close to ready to go.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48159