[PATCH] D128677: [clangd] Add support for generating #import edits

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

we should have tests in `clang/unittests/Tooling/HeaderIncludesTest.cpp` and the commit itself should be tagged as `[clang][Tooling]` rather than `[clangd]`.



================
Comment at: clang-tools-extra/clangd/Headers.h:250
+  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader,
+                                  bool ViaImport) const;
 
----------------
again it's better to have an enum here.


================
Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:253
+                                               llvm::StringRef Symbol,
+                                               bool ViaImport) const {
   Fix F;
----------------
again can we rather pass the directive enum around?


================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:74
+  llvm::Optional<tooling::Replacement>
+  insert(llvm::StringRef Header, bool IsAngled, bool IsImport = false) const;
 
----------------
rather than a boolean flag, can you introduce an `enum IncludeDirective { Include, Import };` and default it to `Include` here?


================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:86
 private:
   struct Include {
     Include(StringRef Name, tooling::Range R) : Name(Name), R(R) {}
----------------
we also need to preserve includedirective here now. as the behaviour of `insert` is to return none iff it's exactly the same spelling. so if there's a `#include "foo.h"` and we want to insert `#import "foo.h"` it shouldn't fail.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128677/new/

https://reviews.llvm.org/D128677