[PATCH] D157905: [include-cleaner] Filter references to identity macros

[Sam McCall via Phabricator via cfe-commits]

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