[PATCH] D92874: [clangd] Validate clang-tidy Checks in clangd config.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:53
 namespace clang {
+namespace tidy {
+/// Returns if a clang tidy \p Check has been registered.
----------------
nit: if this is in the clangd code, it should be in the clangd namespace (isRegisteredTidyCheck or so).


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:58
+  assert(!Check.contains('*') && "isRegisteredCheck doesn't support globs");
+  assert(Check.trim().size() == Check.size() &&
+         "Check has trailing or leading whitespace");
----------------
these asserts seem to violate principle-of-least-surprise - why can't `isRegisteredCheck("-* ")` just return false?


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:26
 
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "CompileCommands.h"
----------------
njames93 wrote:
> sammccall wrote:
> > This is a pretty weird place to depend on clang-tidy.
> > Can we move the "is this a clang-tidy check name" function to somewhere more clang-tidy related, like `TidyProvider.h` or even `clang-tidy/ClangTidyModuleRegistry.h`? ("isRegisteredCheck")
> Moving to `ClangTidyModuleRegistry.h` will still require this include.
> Moving the function to `TidyProvider.h` may have a case, but as TidyProvider doesn't use the function I'm not sure it belongs in there.
Yeah, the include is unfortunate but I think it's an improvement over having the implementation details in the code.

TidyProvider.h - we could rename the header to `Tidy.h` if you think it's important - I can live with the naming discrepancy but think we should group clang-tidy-config related stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92874