[PATCH] D123488: [clangd] IncludeCleaner: Add filtering mechanism

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:105
+
+    /// IncludeCleaner will not diagnose usages of these headers matched by
+    /// these regexes.
----------------
Config is supposed to be cheap to create.
The regex should be created during compilation and shared across all usages.

Maybe store criteria here as `vector<function<bool(StringRef)>>` here and have each function capture a `shared_ptr<vector<Regex>>` by value?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:524
+        diag(Warning,
+             llvm::formatv("Incorrect regular expression: {0}", *HeaderPattern)
+                 .str(),
----------------
nit: invalid, not incorrect (incorrect suggests it's a regular expression that expresses the wrong condition)


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:529
+      }
+      Out.Apply.push_back([&CompiledRegex](const Params &, Config &C) {
+        C.Diagnostics.Includes.IgnoreHeader.emplace_back(
----------------
you're capturing a reference to a local variable


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:531
+        C.Diagnostics.Includes.IgnoreHeader.emplace_back(
+            std::move(CompiledRegex));
+      });
----------------
Each time the resulting fragment is evaluated to create a config, we're going to move out of `CompiledRegex`. After the first time, there's going to be nothing there to move

(apart from the local-variable thing)



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:237
+    struct IncludesBlock {
+      /// Regexes that will be used to avoid diagnosing certain includes. These
+      /// regexes are anchored on the right and will be matched against the
----------------
... as unused or missing.

Second sentence is a bit technical.
"These can match any suffix of the header file in question."?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:265
+  for (auto &Matcher : IgnoreHeader) {
+    if (Matcher.match(Inc.Resolved)) {
+      dlog("{0} header is filterred out", FE->getName());
----------------
what are we doing about slashes?
As written, I think we're matching the regex against native paths, which doesn't really make sense for checked-in configs.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:266
+    if (Matcher.match(Inc.Resolved)) {
+      dlog("{0} header is filterred out", FE->getName());
+      return false;
----------------
filterred -> filtered

not enough context in this dlog to be useful I think: filtered out from what?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123488