unterumarmung wrote: Thanks for taking a look at this at the high level — I appreciate it.
Just to be clear: this is opt-in behavior. Nothing changes unless a project enables `FragmentIncludes` in its config. ### 1) Include ordering Yeah, this can be annoying. But compared to the current state, it’s still a big improvement for projects that have fragments. Right now the realistic options are either: - add `// IWYU pragma: keep` on includes that are only needed because of the fragment, or - don’t run the check (or don’t trust it) on such code. Both are painful at scale. In practice, moving the fragment include out of the “main include list” is usually much easier than sprinkling `pragma: keep` on a bunch of parent includes. And for TableGen in particular, from what I’ve seen those `.inc` includes are often already placed away from the top include block, precisely because they have ordering requirements. So while this doesn’t *solve* ordering, it makes the tool usable in cases where it currently isn’t. ### 2) User confusion Agreed. If the tool keeps inserting includes that aren’t obviously referenced in the parent file, that’s frustrating. One idea could be an optional mode to annotate such includes, e.g. something like: ```cpp #include "Foo.h" // needed by <fragment> ``` (Not proposing the exact spelling here.) But having an *optional* “explain why” hook would help reduce confusion. ### 3) “Fix it in the generator” In an ideal world, yes — generated fragments would be self-contained and this problem disappears. The reason I’m pushing this on the tooling side is that I’m not sure it’s feasible for TableGen (or at least not in the near term), and users still need a way to run include-cleaner correctly today. So the intent isn’t to shift responsibility by default; it’s to provide an opt-in escape hatch when changing the generator output isn’t realistic. Also: IWYU has pragmas for “associated” headers that can help in some setups, but include-cleaner doesn’t support that today, and (from my experience) pragmas end up being harder to maintain than a single config entry. More generally, I’d like LLVM tooling to work well on LLVM-style code when it makes sense and doesn’t fight existing practices. This is a relatively small change that makes include-cleaner usable on TableGen-heavy codebases. That said, I’m not claiming this is the best possible design. If you have an alternative approach that avoids the ordering/confusion pitfalls while still handling non-self-contained fragments, I’d be happy to adjust the direction. https://github.com/llvm/llvm-project/pull/180282 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
