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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+    for (auto &Option : llvm::reverse(OptionStack))
+      Opts.mergeWith(Option, ++Order);
+  };
----------------
sammccall wrote:
> njames93 wrote:
> > sammccall wrote:
> > > This order business is a real tangle, and looks like legacy to me (qualified vs unqualified names).
> > > 
> > > I don't really think we want to use/expose it beyond the requirement that we don't change the meaning of existing sets of `.clang-tidy` config files.
> > > And I don't think we want to bother teaching clangd's config system about it.
> > > 
> > > So I'd suggest a hack/simplification:
> > >  - we remove the `order` parameter from TidyProvider
> > >  - here in provideClangTidyFiles, we just use a local Order which counts up from 1. This preserves the relative precedence of .clang-tidy files
> > >  - in provideClangdConfig, we always set the order to max_uint (always wins)
> > >  - if we ever add checkoptions to any other providers, we set the order to 0 (always loses)
> > >  - combine() doesn't touch order
> > > 
> > > This *does* duplicate the fact that .clangd config is stacked on top of .clang-tidy in ClangdMain, but in a way that only matters with disputes between qualified/unqualified names.
> > That approach is a simplification but it only makes sense in how we are using the providers in clangd. Downstream someone may want to add another provider that may get ran after `.clangd` providers. Forcing `.clangd` to have the highest priority will result in unexpected behaviour in that instance.
> > I'm happy to go with your approach if you don't see it as a blocking issue.
> Any arbitrary fixed number seems like it should work here - we could have .clangd use priority 10000 and then people could use a range higher or lower if they want to support this.
> 
> Mostly it seems like this is probably a theoretical issue only and we shouldn't let it dominate the design. Customizing check options is somewhat rare, config inheritance is rare, getLocalOrGlobal is rare, downstream modifications are rare, etc. I'd rather have the simple version and add the complexity if it causes practical problems.
> 
> (If we really need to solve this at some point, we can probably do it non-intrusively by having combine() lower the priority of all config options by a bunch after each step. We'd need to make priority signed though...)
> (If we really need to solve this at some point, we can probably do it non-intrusively by having combine() lower the priority of all config options by a bunch after each step. We'd need to make priority signed though...)
Or just give everything a high priority as default




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029