unterumarmung wrote: Thanks for the context.
> FYI, there is lack of review capacity in clangd > https://discourse.llvm.org/t/help-needed-with-clangd-maintenance/89820. And > from what I've seen non-trivial feature patches can take several month before > final approval. Understood (and yeah, that’s unfortunate). I’d like to start helping with reviews once I’ve built up more context in the clangd area. For this change specifically, landing it in clangd isn’t a hard requirement for me right now. The behavior can be disabled via config, and the clang-tidy check can remain the source of truth in the meantime. > `include-cleaner` generally doesn't align with LLVM guidelines: > https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible. I don’t think `include-cleaner` is inherently at odds with the LLVM coding standards. 1. The guidelines explicitly say: > You must include all of the header files that you are using — you can include them either directly or indirectly through another header file. So IWYU-style direct includes are allowed. The guidelines don’t require IWYU, but they don’t forbid it either. 2. IWYU itself actually encourages forward declarations where appropriate (and not replacing them with includes): https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md#why-forward-declare AFAIK `misc-include-cleaner` does not try to replace existing forward declarations with includes (it mostly focuses on pruning / adding missing includes), but I might be mistaken here. In any case, it would be useful long-term if `include-cleaner` could also suggest forward declarations in cases where that’s a better fit than adding an include. 3. There has been prior LLVM community interest in IWYU-style cleanup, and it was generally received positively (including reports of compile-time improvements): https://discourse.llvm.org/t/include-what-you-use-include-cleanup/5831 So I think improving include-cleaner’s applicability (including TableGen-generated fragments and, longer-term, forward-decl-friendly behavior) is a reasonable direction. Whether LLVM enables it broadly is a separate decision, but this patch removes a practical blocker. > Could you describe more precise how it was before and how it is now so we can > see real-world benefits. Sure. The issue shows up with TableGen-generated fragments (e.g. `*.inc`) that rely on includes from the parent TU rather than having their own. Before this patch, the generated `.inc` files are effectively analyzed as if they were standalone. Since they typically contain no includes and depend on the parent file’s includes, `misc-include-cleaner` can’t reliably attribute symbol usage coming from the fragment back to the parent. As a result, it may conclude that some includes in the parent are “unused” and suggest removing them, even though they are required for the fragment to compile. With this patch, configuring `FragmentIncludes` to `".inc"` in `.clang-tidy` makes the `.inc` files be treated as fragments of the including file for analysis. That means symbol usage originating in the `.inc` fragment is accounted for when deciding which includes are needed in the parent. In practice, the check stops suggesting removal of includes that are required due to the fragment content. In my pet project, this made `misc-include-cleaner` behave correctly on all TableGen-generated includes: it preserved the needed includes in the parent and, when appropriate, suggested genuinely missing includes based on the combined view of the parent plus fragment. --- This is just my speculative take: IWYU-style hygiene has some nice side benefits — dependencies become clearer, you get fewer “works by accident” transitive includes, and builds can scale better. My guess is LLVM’s guidelines stayed permissive mostly because we didn’t have solid tooling to enforce IWYU reliably across the whole project. As `include-cleaner` gets better, it feels a lot more realistic to apply (at least parts of) IWYU gradually where it makes sense. https://github.com/llvm/llvm-project/pull/180282 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
