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