[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:456
+  auto IncludeStyle =
+      clang::format::getLLVMStyle().IncludeStyle;
+  auto DefaultStyle = clang::format::getStyle(
----------------
creating a copy of LLVM style unnecessarily all the time is not really great, can you move this into the failure case instead?

also you can drop the `clang::` here and elsewhere, as this code is already part of `clang::` namespace.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:457
+      clang::format::getLLVMStyle().IncludeStyle;
+  auto DefaultStyle = clang::format::getStyle(
+      clang::format::DefaultFormatStyle, MainFile->getName(),
----------------
as mentioned above we also need to make sure we're passing the relevant VFS instance inside the source manager, rather than using the real file system (as some clients rely on the VFS).


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:458
+  auto DefaultStyle = clang::format::getStyle(
+      clang::format::DefaultFormatStyle, MainFile->getName(),
+      clang::format::DefaultFallbackStyle);
----------------
s/MainFile->getName()/AST.tuPath()/

to be consistent with other places.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:460
+      clang::format::DefaultFallbackStyle);
+  if (!DefaultStyle.takeError()) {
+    IncludeStyle = DefaultStyle->IncludeStyle;
----------------
can you also `elog` this error? as it should be rare and when this goes wrong, having this mentioned in the logs are really useful for debugging (since the failure is actually outside of clangd, it usually means a malformed config file somewhere)


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:426
+    void foo() {
+      ^b();
+    })cpp");
----------------
nit: instead of using a point, can you use a range here instead (i.e. `[[b]]`)? afterwards you can have a `FileRange` pointing at both offsets, rather than relying on the length of the identifier.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:446
+    BDecl = CandidateDecl;
+    include_cleaner::Symbol B{*D};
+    auto Offset = positionToOffset(MainFile.code(), MainFile.point());
----------------
rest of the code here doesn't really belong to the for loop, can you take them out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143496