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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 19 16:05:44 PST 2020


sammccall added a comment.

Thanks, this looks massively simpler.
It seems clear that we get config by applying a sequence of strategies in order, and these strategies (e.g. .clang-tidy files, config, disabled checks) are mostly independent.
So I have a suggestion for expressing this composition more directly in clangdmain, details in the comments.



================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34
+/// removes any checks known to cause issue in clangd.
+class CheckAdjustingProxyTidyProvider : public tidy::ClangTidyOptionsProvider {
+public:
----------------
it'd be nice if this class could focus on a single responsibility (adapting between interfaces) rather than also trying to apply defaults.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:20
+/// The base class of all clang-tidy config providers for clangd.
+class TidyProvider {
+public:
----------------
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?


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:24
+  /// ClangTidyContext which delegates its option sourcing to this class.
+  virtual std::unique_ptr<tidy::ClangTidyOptionsProvider> getInstance();
+
----------------
This is just adapting this interface to another one, it doesn't need to be virtual (and in fact could be a free function...).

Maybe call it `asClangTidyOptionsProvider` or so? The current name makes it sound like some kind of singleton.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:32
+/// A TidyProvider that uses a Threadsafe Filesystem to traverse up the
+/// directory of a file looking for .clang-tidy configuration files.
+class ThreadSafeFileTidyProvider : public TidyProvider {
----------------
these subclasses are only constructed and used through the TidyProvider interface. I think they can be moved into the impl file and just expose factories.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:45
+  tidy::ClangTidyOptions getOptionsNoOverride(llvm::StringRef FileName);
+  void addOverrideChecks(tidy::ClangTidyOptions &Opts);
+
----------------
It'd be nice to be able to customize the set of checks that are disabled. In our hosted environment, when we find a check that is crashing in production, we disable it with a command-line flag (but without a new release). This flag needs to be propagated through here somehow...

Of course if tidy providers are easy to compose, we can just add our own for this.


================
Comment at: clang-tools-extra/clangd/unittests/TestTidyProvider.h:15
+
+class TestClangTidyProvider : public TidyProvider {
+public:
----------------
(I think it's reasonable to put this trivial implementation into TidyProvider.h as `FixedTidyProvider` or so, to avoid the extra test-only files: the implementation is trivial and there's it does *make sense* as a production provider)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029



More information about the cfe-commits mailing list