njames93 marked 13 inline comments as done. njames93 added inline comments.
================ 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. ---------------- 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. ================ Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:188 + class DynamicDictParser { + public: ---------------- sammccall wrote: > instead of adding a second class for this, can we reuse DictParser, and > change `unrecognized()` so: > - the callback gets access to the key location and the value > - the callback is responsible for emitting the "unknown key" error if it > wants one > > i.e. the default `unrecognized` handler is: > > ``` > [this](Located<std::string> Key, Node &Value) { Outer->warning("Unknown " + > Description + " key " + *Key); } > ``` > > and we replace it for parsing CheckOptions Not sure I'm a huge fan of that approach. I can't imagine a use case where the 2 dictionary modes will be used at the same time so there isn't a need for it to support both. By that I mean you wont have a dictionary where its expected to have both known and unknown keys. `DictParser` could well be implemented as a a specialisation of `DynamicDictParser` but not much would be gained from that. 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