[PATCH] D111870: [clangd] Add a way to enable IncludeCleaner through config

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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)



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:215
+    /// usage.
+    struct IncludeCleanerBlock {
+      /// When set to Strict, IncludeCleaner will warn about all headers that
----------------
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?




================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:216
+    struct IncludeCleanerBlock {
+      /// When set to Strict, IncludeCleaner will warn about all headers that
+      /// are not used (no symbols referenced in the main file come from that
----------------
s/IncludeCleaner/clangd


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:225
+      /// - None
+      llvm::Optional<Located<std::string>> UnusedHeaders;
+    };
----------------
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.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:140
 
+// Returns the range starting at '#' and ending at EOL.
+clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned Line,
----------------
maybe worth a comment that we don't bother to handle escaped newlines etc


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:147
+
+  // Calculate the offset from beginning of the file, EOL to '#'.
+  llvm::StringRef Prefix = Code.drop_back(Code.size() - HashOffset);
----------------
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(...));
```


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:239
+std::vector<Diag> issueUnusedHeadersDiagnostics(ParsedAST &AST,
+                                                const Config &Cfg,
+                                                llvm::StringRef Code) {
----------------
config is usually passed by TLS rather than explicitly


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:254
+    Diag D;
+    D.Message = "included header is not used";
+    D.Name = "unused-header";
----------------
include at least the basename in the message - this also gets listed in e.g. diagnostics view


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:262
+    D.Fixes.emplace_back();
+    D.Fixes.back().Message = "remove unused header";
+    D.Fixes.back().Edits.emplace_back();
----------------
again, we're not removing the header, we're removing the inclusion
"remove #include directive"?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111870