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