[PATCH] D121593: [clangd][WIP] Provide clang-include-cleaner

Mon Mar 14 09:23:24 PDT 2022

kbobyrev added a comment.

Hi! I'm glad you're excited about IncludeCleaner and having a tool to try it out seems like a cool idea!

> I'm not sure why include-cleaner wasn't integrated in clang-tidy, but I suspect there's a good reason.

The problem with IncludeCleaner as of right now is that it depends on some clangd internals. I've started reducing the dependency recently but there's still some way to go before it can really be used in Clang-Tidy or in external tooling. Detaching the most important primitives from clangd is also some work that is often orthogonal to just getting the feature done but I would agree that it is a good idea and a promising venue for future implementation efforts.

Here's what I did so far:

- https://github.com/llvm/llvm-project/commit/089d9c50b29e8e0eb18884edf17451e11a84a80f
- https://github.com/llvm/llvm-project/commit/46a6f5ae148ae2044f13cddf1bb1498a8bcfb372

Apart from what I've mentioned, Clang-Tidy and clangd have different performance trade-offs and policies. In clangd we need to make everything really fast to keep the editor responsive; in Clang-Tidy we can allow expensive checks and disable them by default/provide guidance on how to do large-scale analysis. It's most likely possible to keep a large chunk of the implementation common and implement adapters for both clangd and Clang-Tidy, each of which will target the specific use-case; but that would require some thought, design and effort on making existing infrastructure commonly available (most likely under `clang/Tooling` where I moved `stdlib` handlers).

I've seen the discussion you have started (https://discourse.llvm.org/t/include-what-you-use-include-cleanup/5831). I'd like to read it thorough-fully and reply but I don't think that would happen until next week (at the earliest).

I left some comments but it's really a quasi-review, not a real one. Unfortunately, I'm mostly not working over the last three weeks for personal reasons, so I'm not as useful or up-to-date as I would usually be. @sammccall is a great person to talk to about these changes until I'm back but I'm expecting to start doing _some_ work reasonably soon and getting back to your discussion and this patch is one of the first things I'd like to do.

================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:183
 add_subdirectory(tool)
+add_subdirectory(include-cleaner)
 add_subdirectory(indexer)
----------------
I think it would be better to just put it into `tool/`.

================
Comment at: clang-tools-extra/clangd/include-cleaner/CMakeLists.txt:6
+set(LLVM_LINK_COMPONENTS
+  support
+  )
----------------
nit: 

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:1
+#include "ClangdServer.h"
+#include "support/Logger.h"
----------------
Please use the LLVM style heading and explain what this file & tooling is about.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:7
+
+struct report {
+  PathRef File;
----------------
nit: naming should be `Report`; Clang-Tidy should catch this.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:9
+  PathRef File;
+  void operator()(llvm::Expected<std::vector<const Inclusion *>> C) {
+    if (C) {
----------------
nit: here and elsewhere there are one-to-three letter variable names and it's hard to read the code without going to the definition; it should probably be documented and expanded for clarity.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:14
+        return;
+      llvm::errs() << "Found " << Added.size() << " unused include"
+                   << (Added.size() == 1 ? " " : "s ") << "in: " << File
----------------
Nit: I had a similar tool and it printed both used and unused includes which I found rather useful; maybe it would be good to include/add an option to report both for clarity.

You don't have to do this but maybe add a FIXME/mention it somewhere.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:25
+
+class NullLogger : public Logger {
+  void log(Level, const char *Fmt,
----------------
This seems to be unused.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:31
+int main(int argc, char **argv) {
+  if (argc < 2) {
+    llvm::errs()
----------------
Normally, we don't use plain `argc` and `argv`. LLVM has a nice [[ https://llvm.org/docs/CommandLine.html | Command Line library ]] (`llvm::cl`), please use it instead. It provides a nice abstraction over the primitives and offers useful diagnostics.

Also, please provide a useful `--help` message through this library, this will be super nice for those without access to sources.

================
Comment at: clang-tools-extra/clangd/include-cleaner/ClangIncludeCleanerMain.cpp:68
+
+  return 0;
+}
----------------
nit: `return 0;` at the end of `int main()` isn't necessary in C++

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121593