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

Wed Nov 4 09:13:39 PST 2020

sammccall added a comment.

Thank you!

This design looks really good, just have some nits on comments, possible simplifications etc.

(Sorry about the delay getting to these, on it now!)

================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:268

+  void checkAdjuster(std::string &Adjuster, const Located<std::string> &Arg,
+                     bool IsPositive) {
----------------
nit: method name is confusing because it uses "check" as a noun where you'd expect it to be a verb (this method validates `Adjuster`)

Maybe `appendTidyCheckSpec`?

================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:272
+    StringRef Str = *Arg;
+    StringRef Type(IsPositive ? "Add" : "Remove");
+    // Don't support negating here, its handled if the item is in the Add or
----------------
nit: I think this code is going to too much trouble to produce a precise error message
- not sure "Add check item can't start with '-'" is clearer than without the word "add". Errors show the error location/range.
- "check name" would be clearer than "check item" (even if wildcards are allowed, IMO)
- it feels a bit weird to be really specific about - and , - these are references to a fixed list of checks, so all that really matters is that it's not the name of a check. I'd consider just returning fixed "Invalid clang-tidy check name" for both cases

================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:275
+    // Remove list.
+    if (Str[0] == '-') {
+      diag(Error,
----------------
Str.startswith("-") - we haven't checked that it's empty (and shouldn't bother IMO)

================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:301
+
+    std::string Checks;
+    for (auto &CheckGlob : F.Add)
----------------
nit: I'd suggest *including" the leading comma in these
e.g. Checks is ",foo,bar". (Maybe call it ChecksSuffix)

Then when applying,
`C.ClangTidy.Checks.append(Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/1 : 0);`

advantages:
- marginally simplifies checkAdjuster (no need to conditionally add the comma)
- the Apply step (which is somewhat performance-sensitive) is at most a single allocation (which is awkward to achieve otherwise)

================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:236-237
+    StringRef Type(IsPositive ? "Add" : "Remove");
+    // Don't support negating here, its handled if the item is in the Add or
+    // Remove list.
+    if (Str[0] == '-') {
----------------
njames93 wrote:
> Is this the direction we want to go, the first draft paper on this seems to suggest so.
Yeah it looks like a good solution to me. It's not as powerful as an ordered list of globs, but it's simple and consistent with the other settings we have. `.clang-tidy` is still the right thing for full control.

================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:178
+
+  /// This section controls how clang-tidy will run over the code base
+  ///
----------------
nit: drop "this section" and add trailing period, consistent with other sections.

I think "code" would be clearer than codebase.

================
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.
----------------
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)

================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:184
+    llvm::Optional<Located<bool>> Enable;
+    /// List of checks to enable or disable, can use wildcards.
+    /// \ref Add checks get ran first, then \ref Remove checks. This lets you
----------------
nit: we should document each separately.

e.g. "/// List of checks to enable - can be wildcards like "readability-*".

================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:185
+    /// List of checks to enable or disable, can use wildcards.
+    /// \ref Add checks get ran first, then \ref Remove checks. This lets you
+    /// enable all checks from a module apart from a few specific checks,
----------------
nit: "get ran" is confusing - this doesn't affect the order in which checks *run*, just which overrrides which.
And this text/example could probably be a little more compact.

I'd suggest:

```
List of checks to disable.
Takes precedence over Add. To enable all llvm checks except include order:
  Add: llvm-*
  Remove: llvm-include-onder
```

================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:188
+    /// Example:
+    ///   Add: llvm-
+    ///   Remove: llvm-include-order
----------------
should this be llvm-*?

================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:188

+  class DynamicDictParser {
+  public:
----------------
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

================
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;
----------------
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

================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:239
+    if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
+      Str = S->getValue(Buf).trim();
+    else if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
----------------
not sure why we're trimming here

================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:246
+    }
+    if (Str.equals_lower("true"))
+      return Located<bool>(true, N.getSourceRange());
----------------
Technically we're probably supposed to accept
`y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF`
(https://yaml.org/type/bool.html)

I'd be happy accepting y/n/yes/no/true/false/on/off case-insensitively though :-)

================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:251
+    bool Result;
+    if (!Str.getAsInteger(10, Result))
+      return Located<bool>(Result, N.getSourceRange());
----------------
conversely, not sure why we'd accept a number here

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90531