sammccall added inline comments.
================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:27 + Symbol(Decl &D) : Storage(&D) {} + Symbol(tooling::stdlib::Symbol S) : Storage(S) {} + ---------------- Doh, I was skimming and missed that you removed the `Location` concept and folded stdlib handling into `Symbol`. `tooling::stdlib::Symbol` only captures top-level symbols, so we've thrown away the identity of the actual symbol being accessed. This is not a complete disaster, but I think it causes problems, and I think we should be very deliberate and consistent about what information we preserve vs throw away. Some examples of how this might bite us: - distinguishing between macros/decls is non-uniform (need to check separately for decl vs macro vs stdlib-decl vs stdlib-macro) - can't treat different types of symbols e.g. constructors vs classes differently (you suggested elsewhere we could check the kind of targetdecl to implement external policies) - can't treat nested classes differently (i.e. must treat `std::vector` and `decltype(myvec)::iterator` the same). - can't print the name, e.g. the fixit hint for `auto x = TimePoint<>::min()` becomes `include header <chrono> for 'std::chrono::time_point'` instead of `include header <chrono> for 'std::chrono::time_point<...>::min()'` - can't insert forward declarations with a code action (legal for c symbols) - harder to understand/debug (is this an implicit reference to a copy constructor, move constructor, conversion operator, or the type itself? where *are* all the redecls? is this misclassified user code, such as a trait specialization)? 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