kadircet wrote: I didn't look at the details, because I have couple concerns about the high level approach here: 1. Include-ordering: Non-self contained fragments requires all of their dependencies to be included before them. Current approach doesn't guarantee that (and I am not sure how we can easily). I would like to make sure we don't run into a situation where people also need to add clang-format blocks all over their code. 2. Confusion for the user: When certain includes aren't cleaned-up or keep getting inserted without an obvious dependency in the source code, users are likely to get frustrated. 3. Pushing responsibility to user code: Most of the include-fragments are generated code, i.e. there's a single place where all of this can be fixed by changing the code generation logic. I believe pushing that "fix" to all the leaves, rather than handling them centrally is the wrong call (and creates at least 2 more problems mentioned above).
--- also some drive-by comments, in case people are interested in chasing those directions > AFAIK misc-include-cleaner does not try to replace existing forward > declarations with includes (it mostly focuses on pruning / adding missing > includes) That's true, but there's some more to that. Yes, include-cleaner will treat "forward declarations" as providers, but that'll result in misbehavior for reverse-dependencies that need the complete type, e.g. if you need `llvm::StringRef` and there's an include in your file that forward declares it, include-cleaner **will not** insert the relevant include. Things get more complicated when the usage of a type isn't explicitly spelled, but happens through member calls (on possibly aliases). So include-cleaner only tries to **not make things worse** in presence of forward declarations, rather than aggressively focusing on hygiene. > it would be useful long-term if include-cleaner could also suggest forward > declarations I believe this is (or at least was) one of the fundamentally difficult things in clang-based source tooling. It's really hard to figure out if a complete type is needed or not. We deliberately made the call to assume no forward decls in include-cleaner purely because of that to cut the extra complexity. You can compare the project with https://github.com/include-what-you-use/include-what-you-use to see how big of an impact it has in the code size. That being said; maybe things changed in clang (or can be changed) to make this more tractable in the clients. https://github.com/llvm/llvm-project/pull/180282 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
