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
