[PATCH] D90531: [clangd] Add clang-tidy options to config

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:78
+    std::string Checks;
+    std::vector<std::pair<std::string, std::string>> CheckOptions;
+  } ClangTidy;
----------------
njames93 wrote:
> sammccall wrote:
> > I think this should be a StringMap<string>
> > 
> > It makes sense to use a vector-of-pairs in ConfigFragment to preserve the Located information for keys, but we don't need to do that in Config.
> If we do use a `StringMap<string>`, whats the policy when multiple fragments specify the same option.
> Currently at the Fragment level there is logic that ignores duplicated keys, but when compiling the fragments, should we give priority to keys declared in earlier or later fragments. In any case would it also be wise to warn about that.
We merge with the later fragments taking priority. So in this case each fragment writes its keys into the single stringmap, replacing whatever is there.

(Most common case of later fragments is subdirectory overriding parent, or user config overriding in-tree).

I don't think we need to warn for this - does clang-tidy warn if there are multiple options sources configured and one overrides a setting from another? This is the same thing.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:183
+  struct ClangTidyBlock {
+    llvm::Optional<Located<bool>> Enable;
+    /// List of checks to enable or disable, can use wildcards.
----------------
njames93 wrote:
> sammccall wrote:
> > njames93 wrote:
> > > sammccall wrote:
> > > > I wonder if it's worth having this when we support `Remove: *`.
> > > > 
> > > > technically this does something slightly different:
> > > >  - it still runs the machinery just with no actual checks
> > > >  - you can enable: true later to override it, without losing the previously configured list of checks
> > > > 
> > > > Is one of these important? Is there some other benefit?
> > > > (I'm not opposed to having this if you want it, but I'm curious)
> > > I'm not 100% sure what you are asking here.
> > I'm asking whether we really need `Enable`, or whether we should remove it and recommend `Remove: *` to disable the checks.
> > 
> > If there isn't a clear reason we need it, my preference is to omit it for simplicity (we can add more setting later, but it's harder to remove them).
> > 
> > I don't feel strongly though, this is up to you.
> If we use `Remove: *` when it comes to actually implementing this, it would be wise to disable running clang-tidy if there are no checks enabled. But yes, I do agree that it could be removed at first, then added at a later date if needs must.
This sounds like a good idea to me. This would happen in `ParsedAST::build`.
If the loop through CTChecks never actually manages to call registerPPCallbacks/Matchers, then we can skip the `matchAST` call.

I'll send a patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90531