[PATCH] D111870: [clangd] Add a way to enable IncludeCleaner through 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.

Awesome, let's ship it!
Only nits on the text, feel free to ignore any you don't like



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:213
 
+    /// Controls how clangd will treat and warn users on suboptimal include
+    /// usage.
----------------
brevity: consider "correct" or "diagnose" instead of "treat and warn users on".
specificity: consider "unnecessary #include directives" instead of "suboptimal include usage".


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:217
+    /// When set to Strict, clangd will warn about all headers that are not used
+    /// (no symbols referenced in the main file come from that header) in the
+    /// main file but are directly included from it.
----------------
We're glossing over the key idea ("come from") here, and mixing what the Strict policy is about with what the feature is about. I think we can be a bit more precise.

I'd add a second sentence to the first paragraph, elaborating a little:
```
clangd can warn if a header is `#included` but not used, and suggest removing it.
```

And then we can define the Strict policy, and mention its limitations.

```
Strict means a header is unused if it does not *directly* provide any symbol used in the file.
Removing it may still break compilation if it transitively includes headers that are used.
This should be fixed by including those headers directly.
```


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:220
+    ///
+    /// FIXME(kirillbobyrev): Removing that header might break the code if it
+    /// transitively includes used headers and these headers are not included
----------------
I think at least the "fixme" part isn't suitable for public docs.
We also don't mention what we want to do instead.

I'd suggest:
 - move this to the part of the code that generates fixes
 - mention that we if it's transitively used, we could suggest replacing it with the needed `#include`s instead (I think this is actually pretty feasible...)
 - while there, maybe also add a FIXME that we should suggest adding IWYU pragma export/keep as an alternative fixes (once we honor that)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:251
+    Diag D;
+    D.Message = llvm::formatv("included header {0} is not used", Inc->Written);
+    D.Name = "unused-includes";
----------------
This is totally up to you, but wanted to be explicit with what I meant about basename...

If the path is long, this can be: `included header this/is/a/long/path/to/some/header.h is not used`
Depending how you feel about having to visually scan past the path to get to the end of the sentence, you might choose to trade precision for brevity: `included header header.h is not used`.

This is `llvm::sys::path::filename(stripHeaderQuotes(Inc->Written), posix)`, with stripHeaderQuotes to be written.

Again, I don't feel strongly, your call


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:215
+    D.Message = "Included header is unused";
+    D.Name = "clangd-include-cleaner";
+    // FIXME: This range should be the whole line with target #include.
----------------
kbobyrev wrote:
> sammccall wrote:
> > sammccall wrote:
> > > Source should be `Clangd` I think (our first?).
> > > 
> > > Name should be something like "unused-include": this doesn't need to echo the tool name.
> > clangd-unused-header -> unused-header
> > 
> > (there's a test, but it's stale)
> what do you mean by "stale"?
You'd updated the diagnostic name, but not the suppression check. I was going to suggest a test for suppression, and realized you had one, it was also testing the old name though.

LG now!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111870