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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
