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