This is useful for dogfooding the feature and spotting bugs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/Config.h | ||
---|---|---|
90 | you've accidentally split this comment from its decl | |
91 | structure, as discussed offline... "include cleaner" refers to a set of features to do with header hygiene, related but I think we still want to configure them individually. "Unused headers" is the feature we're trying to configure, so I think it's a key rather than a value. Its value could be a boolean, (and configure other aspects separately), or we could make it an enum so we can choose a policy at the same time. So I'd probably call this UnusedInclude: Strict | |
94 | don't group with the generic suppression bits | |
clang-tools-extra/clangd/ConfigFragment.h | ||
213 | This is where the documentation for the feature goes :-) | |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
242 | Source should be Clangd I think (our first?). Name should be something like "unused-include": this doesn't need to echo the tool name. | |
243 | remove FIXEDME | |
248 | This doesn't handle hitting EOF correctly, and also gets encodings wrong (should be UTF-16 code units, unless we're in another mode). I think you want: D.range.end.character = lspLength(Code.drop_front(Inc->HashOffset).take_until([](char C) { return c == '\n' || c == '\r'; })); You could make this a function in SourceCode.h if you like. | |
253 | No need to explain the ranges thing, this is ubiquitous and documented on the structs. | |
256 | no-op | |
257 | don't do this in the loop | |
262 | D.Tags.push_back(Unneccesary) This produces nice rendering in vscode | |
clang-tools-extra/clangd/IncludeCleaner.h | ||
67 | nit: llvm::StringRef | |
clang-tools-extra/clangd/ParsedAST.cpp | ||
380 | You're bypassing this logic, which is OK, but you'll need to duplicate it somewhere. (Probably wherever is looking at cfg anyway) | |
519 | You're missing tests for this functionality | |
524 | If/when we support other policies, the issueUnusedIncludesDiagnostics function will need to look at the current policy, so you might want to move this check there already. (We also need to check for suppression, as mentioned above) | |
525 | you're copying all the diags here I don't mind the temp vector to keep the signature nice, but Result.Diags->insert(Result.Diags->end(), make_move_iterator(NewDiags.begin()), etc)? |
[clangd] IncludeCleaner: Implement more complicated rules for considering enums forward declarations "used"
Mostly dumb nitpicks because this is user-facing magic strings... let's chat before doing another round so I'm not wasting your time.
You've done some renaming from UnusedInclude -> UnusedHeaders.
I feel bad because I did say in the last round
"Unused headers" is the feature we're trying to configure
but I wasn't intending to be precise and actually think the old name is better (some discussion below)
clang-tools-extra/clangd/ConfigFragment.h | ||
---|---|---|
215 | I'm not sure we need a separate block for this. The main cost is an extra line in the config file, and it makes the "include cleaner" name user-facing, so it's an extra concept people have to learn. Do you think we'll need a lot more than UnusedHeaders/MissingHeaders? | |
216 | s/IncludeCleaner/clangd | |
225 | I wonder whether "UnusedIncludes" would be clearer, especially given the pairing with "MissingHeaders" (consider confusion with pp_file_not_found) vs "MissingIncludes". Conceptually it's an unused edge rather than node, but mostly i'm just thinking "MissingIncludes" more clearly describes the nature of the fix. | |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
149 | maybe worth a comment that we don't bother to handle escaped newlines etc | |
156 | This code is making me a bit dizzy. I get the idea that we're using the precise HashOffset while exploiting the fact that we know the line number to avoid counting lines. But I'm not sure it's worthwhile, we're near the top of the file after all... What about just Result.end = result.start = offsetToPosition(code, HashOffset) Result.end.character += lspLength(Code.drop_front(HashOffset).take_until(...)); | |
237 | config is usually passed by TLS rather than explicitly | |
242 | clangd-unused-header -> unused-header (there's a test, but it's stale) | |
252 | include at least the basename in the message - this also gets listed in e.g. diagnostics view | |
260 | again, we're not removing the header, we're removing the inclusion |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
---|---|---|
242 | what do you mean by "stale"? |
Awesome, let's ship it!
Only nits on the text, feel free to ignore any you don't like
clang-tools-extra/clangd/ConfigFragment.h | ||
---|---|---|
213 | brevity: consider "correct" or "diagnose" instead of "treat and warn users on". | |
217 | 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. | |
220 | I think at least the "fixme" part isn't suitable for public docs. I'd suggest:
| |
clang-tools-extra/clangd/IncludeCleaner.cpp | ||
242 | 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! | |
251 | 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 This is llvm::sys::path::filename(stripHeaderQuotes(Inc->Written), posix), with stripHeaderQuotes to be written. Again, I don't feel strongly, your call |
you've accidentally split this comment from its decl