[PATCH] D90531: [clangd] Add clang-tidy options to config

[Sam McCall via Phabricator via cfe-commits]

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