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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits