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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
