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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly LG. some small changes.



================
Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:261
 
   if (Symbol.empty())
+    F.Message = llvm::formatv("{0} {1}",
----------------
nit: `llvm::StringLiteral DirectiveSpelling = Directive == tooling::IncludeDirective::Include ? "Include" : "Import";` and use that inside formatv calls


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:340
+  Edit = insert("\"header.h\"", tooling::IncludeDirective::Import);
+  EXPECT_TRUE(Edit);
+  EXPECT_EQ(Edit->newText, "#import \"header.h\"\n");
----------------
nit: ASSERT_TRUE, here and above.


================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:47
 
+enum IncludeDirective { Include, Import };
+
----------------
this will leak enum values to the whole `clang::tooling` namespace. can you make this an `enum class IncludeDirective` instead (sorry for forgetting that in the initial comment)


================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:77
+  insert(llvm::StringRef Header, bool IsAngled,
+         IncludeDirective Directive = IncludeDirective::Include) const;
 
----------------
i don't think we have many callers here. can we just update them instead of defaulting to `Include` here?


================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:79
 
   /// Removes all existing #includes of \p Header quoted with <> if \p IsAngled
   /// is true or "" if \p IsAngled is false.
----------------
s/#includes/#includes and #imports/


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:300
+                      Offset, std::min(Line.size() + 1, Code.size() - Offset)),
+                  Matches[1] == "include" ? tooling::IncludeDirective::Include
+                                          : tooling::IncludeDirective::Import),
----------------
i know this is not possible today, but just to be safe, can we actually check for `import` here and default to `include` ?


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:354
   auto It = ExistingIncludes.find(IncludeName);
   if (It != ExistingIncludes.end())
     for (const auto &Inc : It->second)
----------------
nit: can you put some braces around the outermost `if`, as the statements inside are getting long.


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:378
   assert(InsertOffset <= Code.size());
   std::string NewInclude =
+      Directive == IncludeDirective::Include
----------------
nit:
```
llvm::StringLiteral DirectiveSpelling = Directive == IncludeDirective::Include ? "include" : "import";
std::string NewInclude = llvm::formatv("#{0} {1}\n", DirectiveSpelling, QuotedName); // this already has an implicit conversion operator to std::string
```


================
Comment at: clang/unittests/Tooling/HeaderIncludesTest.cpp:65
 
+TEST_F(HeaderIncludesTest, InsertImportWithSameInclude) {
+  std::string Code = "#include \"a.h\"\n";
----------------
can you also add a removal test?

i know we didn't change the implementation, but today there's nothing checking the behaviour. so explicitly encoding the expectation here, especially for the case where we have both #include and #import with same spelling is useful. btw is it likely that `#include "foo.h"` and `#import "foo.h"` can refer to different physical headers (e.g. due to framework handling and whatnot)?

we're not changing the behaviour today, but if there's a difference, this is likely a bug (that already exists), so we should leave a fixme (or address it here if you got time, by introducing a IncludeDirective parameter into remove. updating users is likely harder in this case, as everyone needs to propagate/store the directive during their analysis).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128677