sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! Just some simplifications and doc nits left, then please go ahead and
land
================
Comment at: clang-tools-extra/clangd/Config.h:74
+
+ // Configures what clang-tidy checks to run and options to use with them.
+ struct {
----------------
nit: we're using triple slash comments for these...
================
Comment at: clang-tools-extra/clangd/Config.h:77
+ bool Enable = true;
+ std::string Checks;
+ std::vector<std::pair<std::string, std::string>> CheckOptions;
----------------
`Enable` is trivial enough to go without documentation, but the format of
`Checks` certainly needs to be documented.
================
Comment at: clang-tools-extra/clangd/Config.h:78
+ std::string Checks;
+ std::vector<std::pair<std::string, std::string>> CheckOptions;
+ } ClangTidy;
----------------
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.
================
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:
> > 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.
================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:188
+ class DynamicDictParser {
+ public:
----------------
njames93 wrote:
> 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.
Yes, they're being used for different purposes, so using the same class isn't
the most expressive.
But this isn't a public API, it's a local helper. The implementation is
nontrivial and almost identical. This is just about reducing the implementation
complexity.
================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:235
+ // Try to parse a single boolean value from the node, warn on failure.
+ llvm::Optional<Located<bool>> scalarBool(Node &N, llvm::StringRef Desc) {
+ llvm::SmallString<256> Buf;
----------------
sammccall wrote:
> can we implement this on top of scalarValue? seems like it would avoid a
> bunch of repetition and the efficiency doesn't seem that important
You've extracted a common getScalar function here instead, I think to avoid
scalarValue copying the string?
But this isn't a hot path, and all the legal values for this string are in SSO
range anyway - can we avoid this extra complexity?
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
