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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

still LG, comments are still confusing me a little



================
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
----------------
kadircet wrote:
> sammccall wrote:
> > 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.
> > 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
> 
> That's absolutely right. The issue here is macro expansion triggers independent of the context, e.g.
> ```
> #include <stdio.h>
> namespace ns { void stderr(); }
> void foo() { ns::stderr(); }
> ```
> 
> here we have a (well two) reference to `stderr` macro from stdandard library, which is not user's intent, but rather a quirk of the language.
Sure, but this code is not valid C++ (since `stderr` is not guaranteed to expand to a single identifier). Is this actually a common/motivating case?


================
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.
----------------
kadircet wrote:
> sammccall wrote:
> > 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?
> > 
> this should've been "false negatives" from the "usedness perspective", we basically drop the reference to `ns::stderr` in above example, because the name is not spelled in the main file (but rather spelled inside a macro body).
And now it's backwards if your perspective is this function itself!

just avoid "false positives"/"false negatives" terminology and describe the actual effect?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157905/new/

https://reviews.llvm.org/D157905