[PATCH] D136293: [IncludeCleaner] Add public API

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 2 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:67
+        llvm::errs() << "returning stdlib\n";
+        return CB(Loc, Symbol(*SS), toHeader(SS->headers()));
+      }
----------------
sammccall wrote:
> (I think the patch is fine, but could use some extra docs/fixmes depending on what the choices are here).
> 
> In general we want a forward declaration in the main file to suppress any include insertion.
> Two obvious ways to achieve that:
> A) walkUsed doesn't report refs of symbols that have a decl in the main file
> B) walkUsed reports the refs, the header list includes the main file, the caller recognizes it when checking if the ref is satisfied
> 
> A) feels more ad-hoc, but seems to work and *does* naturally achieve the nice effect that `#include "foo.h"` is marked as unused if the only used symbols are also forward-declared locally. In this case, maybe add a FIXME for this filtering? Also A seems surprising enough to be worth documenting.
> 
> B) falls more naturally out of the implementation. It looks like  we have a bug here: by bailing out early, we'll omit any forward declarations from the main file. For symbols in `namespace std` we're arguably [within our rights as such decls are UB](http://eel.is/c++draft/namespace.std#1). However such forward decls are legal in C, so maybe we should care after all? In any case, this subtlety deserves some sort of comment.
thanks, i think the conclusion is `B)` here, but definitely didn't notice the implications of early bailout here. adding some comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136293