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