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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
