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