kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/ASTSignals.h:39 + + static Symbol::IncludeDirective preferredIncludeDirective( + llvm::StringRef Filename, const LangOptions &LangOpts, ---------------- could you rather move this to `AST.h`, with some comments explaining what it does ================ Comment at: clang-tools-extra/clangd/ASTSignals.h:41 + llvm::StringRef Filename, const LangOptions &LangOpts, + const IncludeStructure &Includes, ArrayRef<Decl *> TopLevelDecls); }; ---------------- what about accepting an `llvm::ArrayRef<Inclusion> MainFileIncludes` instead of the full `IncludeStructure`, as the former can be constructed a lot more easily. ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:395 + Symbol::IncludeDirective Directive = insertionDirective(Opts); + tooling::IncludeDirective InsertDirective = + Directive == Symbol::Import ? tooling::IncludeDirective::Import ---------------- nit: I'd actually inline this into use-side to not confuse the reader unnecessarily about whether this is marshalling/conversion ================ Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:322 llvm::StringSet<> InsertedHeaders; + auto InsertDirective = Directive == Symbol::IncludeDirective::Import ? + tooling::IncludeDirective::Import : tooling::IncludeDirective::Include; ---------------- again, i'd inline the marshalling ================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:566 if (Preamble) { + Includes = Preamble->Includes; for (const auto &Inc : Preamble->Includes.MainFileIncludes) ---------------- this is a somewhat heavy struct to copy (contains absolute paths to all the includes in the preamble). can you use a pointer here? ================ Comment at: clang-tools-extra/clangd/ParsedAST.h:88 /// (These should be const, but RecursiveASTVisitor requires Decl*). - ArrayRef<Decl *> getLocalTopLevelDecls(); + ArrayRef<Decl *> getLocalTopLevelDecls() const; ---------------- can you rather introduce a new overload that returns `ArrayRef<const Decl*>` ================ Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:87 + HeaderTU.ExtraArgs = {"-xobjective-c++-header"}; + Signals = ASTSignals::derive(HeaderTU.build()); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Import); ---------------- since we have a dedicated interface now, could you please test it instead (same for uses below)? ================ Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:98 + HeaderTU.Code = R"cpp( + #include "TestTU.h" + ---------------- no need for the `#include` here Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139458/new/ https://reviews.llvm.org/D139458 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits