[PATCH] D122677: [prototype] include-cleaner library

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

hi Serge, sorry for the silence here. we'll be moving this towards a production ready version over the next months (also releasing some comments i've forgotten to send, we've already discussed these offline so no need for action).
we'll probably be leaving some todos/fixmes as we go, if you want to help it'd be great to keep an eye on changes going into the clang-tools-extra/include-cleaner directory (i'll also try to add you as a subscriber to them) and mention things you'd be interested in picking up.



================
Comment at: clang-tools-extra/clang-tidy/misc/UnusedIncludesCheck.cpp:60
+      });
+  for (const auto &I : RecordedPP->Includes.all()) {
+    if (!Used.contains(&I)) {
----------------
i guess one annyoing thing we're missing here is communication of `IWYU export / keep` like pragmas. It feels like handling of such will now be distributed across all users, which feels unfortunate.
maybe we can address that by either completely deleting such headers from `RecordedPP->Includes.all` or having some helper like `shouldKeep` in RecordedPP->Includes (similar to match).


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:50
+  Symbol(NamedDecl *ND) : Target(ND) {}
+  Symbol(const DefinedMacro *M) : Target(M) {}
+
----------------
i think we might want to have some pointers to AnalysisContext owning these DefinedMacros, it's pretty surprising for the user otherwise.


================
Comment at: clang-tools-extra/include-cleaner/lib/Headers.cpp:37
+  case Location::StandardLibrary:
+    // FIXME: some symbols are provided by multiple stdlib headers:
+    //   - for historical reasons, like size_t
----------------
feels like we might need some language-based hints here. like stdio.h is C-like.

for the multiple header cases, i suppose it's fine to put them in an order that we think is "common", while still providing the others. so that unused includes usecase won't annoy folks and missing includes can provide sane defaults.

i am not sure if we care much about "guaranteed umbrella headers" case. but in such a scenario, i guess we'd want a policy, that way missing includes can be suppressed but unused includes can still fire (basically a flip that returns only the specific header vs both specific and umbrella header).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122677