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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:78
+    std::string Checks;
+    std::vector<std::pair<std::string, std::string>> CheckOptions;
+  } ClangTidy;
----------------
sammccall wrote:
> 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.
If we do use a `StringMap<string>`, whats the policy when multiple fragments specify the same option.
Currently at the Fragment level there is logic that ignores duplicated keys, but when compiling the fragments, should we give priority to keys declared in earlier or later fragments. In any case would it also be wise to warn about that.


================
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:
> 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.
If we use `Remove: *` when it comes to actually implementing this, it would be wise to disable running clang-tidy if there are no checks enabled. But yes, I do agree that it could be removed at first, then added at a later date if needs must.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:188
 
+  class DynamicDictParser {
+  public:
----------------
sammccall wrote:
> 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.
Ok I'll update it.


================
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:
> 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?
It's not about that, I'd prefer the warning message to explicitly say it needs a boolean even if they pass something that's not a scalar. Reusing `scalarValue` will give a warning saying `SomeKey should be scalar`. What we actually want is a warning saying `SomeKey should be a boolean`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90531/new/

https://reviews.llvm.org/D90531