sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:37
+namespace {
+bool shouldSkipMacro(const Macro &M) {
+ static const auto *MacroNamesToIgnore = new llvm::StringSet<>{
----------------
shouldIgnoreMacroReference would better distinguish vs macro definition
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:43
+ // This results in surprising behavior from users point of view (we
+ // generate a usage of stdio.h, in places unrelated to standard library).
+ // FIXME: Also eliminate the false positives by treating declarations
----------------
Comment nit: I'm having trouble imagining cases that are actually*unrelated* to
the stdlib.
If `stderr` is an impl-defined macro, then the only way to use the name to
refer to something else is if it's not defined (inside ifndef stderr, or if you
can be sure your TU doesn't include stdio first). Seems implausible...
That said I feel like we've had this conversation before and I've just
forgotten the details.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:44
+ // generate a usage of stdio.h, in places unrelated to standard library).
+ // FIXME: Also eliminate the false positives by treating declarations
+ // resulting from these expansions as used.
----------------
Nit: "false positives" is a little unclear: positives for this function are
negatives for walkAST and could be either for diagnostics.
Also, I think you mean *references* rather than declarations?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157905/new/
https://reviews.llvm.org/D157905
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
