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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
