njames93 marked 2 inline comments as done.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:20
+/// The base class of all clang-tidy config providers for clangd.
+class TidyProvider {
+public:
----------------
sammccall wrote:
> we're using inheritance for a couple of purposes here.
>
> First, giving a common interface to the various types of provider.
> This makes sense, though we use
> `unique_function<ClangTidyOptions(StringRef)>` for this as the type as it's
> essentially just one function.
> This would give us the option of using lambdas instead of having
> TestClangTidyProvider...
>
> The other purpose is composition: ConfigTidyProvider inherits from
> ThreadSafeFileTidyProvider in order to provide both sets of config.
> This is awkward and constraining, e.g. we can't now enable config but not
> `.clang-tidy` (which is the desired config inside google), and more
> immediately the handling of overrides is awkward.
> It seems more natural to have several units and compose them explicitly. The
> most simple way to compose them is have them be functions that mutate a
> ClangTidyOptions, instead of producing one.
>
> So in its minimal form this could look like:
> ```
> using TidyProvider = unique_function<void(ClangTidyOptions&,
> /*Filename*/llvm::StringRef)>;
> TidyProvider combine(vector<TidyProvider>);
> ClangTidyOptionsProvider asClangTidyOptionsProvider(TidyProvider);
>
> TidyProvider provideEnvironment();
> TidyProvider provideDefaultChecks();
> TidyProvider disableUnusableChecks(ArrayRef<std::string> OverrideChecks={});
> TidyProvider provideClangdConfig();
> TidyProvider provideClangTidyFiles(ThreadsafeFS&);
>
> // In ClangdMain:
> TidyProvider Tidy = combine({provideEnvironment(), provideClangTidyFiles(FS),
> provideClangdConfig(), provideDefaultChecks(), disableUnusableChecks());
> ```
>
> What do you think?
That is a much nicer interface
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91029/new/
https://reviews.llvm.org/D91029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
