[PATCH] D139458: [clangd] Full support for #import insertions

[Kadir Cetinkaya via Phabricator via cfe-commits]

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