[PATCH] D91029: [clangd] Implement clang-tidy options from config

[Nathan James via Phabricator via cfe-commits]

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