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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 4 inline comments as done.
njames93 added inline comments.


================
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");
----------------
sammccall wrote:
> these asserts seem to violate principle-of-least-surprise - why can't `isRegisteredCheck("-* ")` just return false?
The reason for this pre condition was because I wanted to assert that the first non whitespace item was not a `-`. But there are better ways around that.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:26
 
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "CompileCommands.h"
----------------
sammccall wrote:
> 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.
That sounds like a refactor that shouldn't live in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92874