sammccall added a comment. This is exciting! We weren't sure how much effort to put into decoupling this from clangd, whether there'd be interest in such a standalone tool. Some thoughts at a high level:
--- Eventually we should separate this from clangd. I don't think it necessarily needs to be right away, but we should avoid hopelessly entangling it. In particular, I don't think we should be going through `ClangdServer`. As well as pulling in ~all of clangd, this is going to spawn threads, write PCHes to disk, and limit flexibility in how we process files. --- > I'm not sure why include-cleaner wasn't integrated in clang-tidy, but I > suspect there's a good reason. The TL;DR is I think it could be, and we should consider whether to do this instead of adding a separate tool. Our initial goal was to get this functionality in clangd. While clangd has support for tidy checks, it's limited: the checks only see AST and PP events from the main file (for performance reasons). This means if you implemented a clang-tidy check in clangd in the obvious way (collect `#include` information in PPCallbacks, query it while traversing) this check would not work in clangd. Instead, we built it into clangd in a way that's aware of our preamble optimization - most of the PP events are reused across reparses. However the capture-PP-info part is fairly simple, and the traverse-AST-and-gather-locations part is independent of clangd. We could "easily" share this part as a library and build a clang-tidy check out of the same core. Advantages to making this a clang-tidy check: - for users: fits into existing workflows and integrations - for maintainers: existing framework takes care of parsing ASTs, emitting diagnostics, applying fixes Advantages to a separate tool: - more control over how code is processed, e.g. traversing headers exactly once - the check abstraction may not be a perfect fit, e.g. maybe produce a report instead of diagnostics, or treatment of alternate fixes - probably some minor efficiency gains as we're not really using matchers etc Overall I suspect making this a tidy check would make it more useful to more people. It would mean sorting out the dependency issue sooner rather than later though. --- I think it's important to have some spelled-out scope of what we want this tool to do and what workflows it's aiming for. - applying fixes vs reporting problems? (If applying, what to do when multiple fixes are possible) - adding vs removing includes, or both? (We do plan to support adding includes in clangd) - being conservative vs being complete (feeling safe to run this unattended is a big feature, c.f. IWYU) - do you run it over files or codebases? (important to consider how headers are treated) - assume "well-behaved" code, or does it try to understand everything? - does it make use of forward declarations, or just includes? It's OK to make some of these configurable, but the answers are important to default behavior and also where effort is focused. Hopefully many of the answers are aligned with what we want for clangd. If not that's OK, but we should plan for this rather than be surprised when it happens :-) ================ Comment at: clang-tools-extra/clangd/CMakeLists.txt:183 add_subdirectory(tool) +add_subdirectory(include-cleaner) add_subdirectory(indexer) ---------------- kbobyrev wrote: > I think it would be better to just put it into `tool/`. LLVM's CMake rules make it hard to have multiple binaries in the same directory, so I think this is correct as-is. ================ Comment at: clang-tools-extra/clangd/include-cleaner/CMakeLists.txt:1 +add_clang_tool(clang-include-cleaner + ClangIncludeCleanerMain.cpp ---------------- Until this is a bit more ready for real use, I think this should be `add_clang_executable` so it doesn't get packaged Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121593/new/ https://reviews.llvm.org/D121593 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
