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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
