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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks! looks amazing, we're missing a little bit of test coverage though



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:280
 
+bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderSpelling) {
+  // Convert the path to Unix slashes and try to match against the filter.
----------------
s/HeaderSpelling/HeaderPath


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:282
+  // Convert the path to Unix slashes and try to match against the filter.
+  llvm::SmallString<64> Path(HeaderSpelling);
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
----------------
s/Path/NormalizedPath


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:414
+
+std::string findResolvedPath(const include_cleaner::Header &SymProvider) {
+  std::string ResolvedPath;
----------------
what about just `resolvedPath`, if you'd rather keep the verb, i think `get` makes more sense than `find`. we're not really searching anything.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:418
+  case include_cleaner::Header::Physical:
+    ResolvedPath = SymProvider.physical()->tryGetRealPathName();
+    break;
----------------
nit: you can directly `return  SymProvider.physical()->tryGetRealPathName();` (same for other 2 cases) and have an `llvm_unreachable("Unknown symbol kind");` after the switch statement.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:421
+  case include_cleaner::Header::Standard:
+    ResolvedPath = SymProvider.standard().name();
+    break;
----------------
in this and the next case we need to trim `<>"`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:430
+
+std::string findSymbolName(const include_cleaner::Symbol &Sym) {
+  std::string SymbolName;
----------------
same as above, either just `symbolName` or `get`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:434
+  case include_cleaner::Symbol::Macro:
+    SymbolName = Sym.macro().Name->getName();
+    break;
----------------
again you can just return here and below


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:434
+  case include_cleaner::Symbol::Macro:
+    SymbolName = Sym.macro().Name->getName();
+    break;
----------------
kadircet wrote:
> again you can just return here and below
`getName` is a StringRef, and unfortunately there are some platforms (like darwin) that don't support implicit conversion from stringrefs to std::string. so can you call `.str()` explicitly in the end?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:754
+  IncludeCleanerFindings Findings;
+  if (Cfg.Diagnostics.MissingIncludes != Config::IncludesPolicy::None ||
+      Cfg.Diagnostics.UnusedIncludes != Config::IncludesPolicy::None) {
----------------
i think for now this should be
```
if (Cfg.Diagnostics.MissingIncludes == Config::IncludesPolicy::Strict ||
  Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Experiment) {
```

otherwise we'll run both legacy and new analysis for `UnusedIncludes == Strict`


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:449
+    include_cleaner::Symbol B{*D};
+    syntax::FileRange BRange{SM, B.declaration().getBeginLoc(), 1};
+    include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
----------------
this is pointing at the declaration inside `b.h` not to the reference inside the main file. are you sure this test passes?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:469
+      $d[[d]]();
+      $f[[f]]();
+    })cpp");
----------------
can you also add a reference (and declaration) for std::vector, and have an IWYU private pragma in one of the headers to test code paths that spell verbatim and standard headers? also having some diagnostic suppressed via `IgnoreHeaders` is important to check


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:481
+  TU.AdditionalFiles["system/e.h"] = guard("#include <f.h>");
+  TU.AdditionalFiles["system/f.h"] = guard("void f();");
+  TU.ExtraArgs.push_back("-isystem" + testPath("system"));
----------------
can you make one of these names qualified? e.g. `namespace ns { struct Bar { void f(); }; }`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143496