[PATCH] D140551: [include-cleaner] Don't count references to operators as uses

[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/WalkAST.cpp:75
+    // doesn't count as uses (generally the type should provide them).
+    // Ignore them by traversing arguments instead of children.
+    for (auto *Arg : S->arguments())
----------------
This comment doesn't mention the callee at all, and the callee is the point.

Maybe: "Don't traverse the callee." and add a newline before the arguments() loop?
(because this comment isn't really related to that part, arguments() is just part of the "everything else" that we're still doing)


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:69
 
+  bool TraverseCXXOperatorCallExpr(CXXOperatorCallExpr *S) {
+    // Operators are always ADL extension points, by design references to them
----------------
hokein wrote:
> sammccall wrote:
> > Rather than override TraverseCXXOperatorCallExpr to do everything *except* call WalkUpFrom, maybe override WalkUpFromCXXOperatorCallExpr to call VisitCXXOperatorCallExpr but not WalkUpFromCallExpr? (With a comment that we don't want to treat operators as calls)
> > 
> > Rather than override TraverseCXXOperatorCallExpr to do everything *except* call WalkUpFrom.
> 
> Ah, not really (it is my fault that I missed to call `WalkUpFrom`, but it is not the key point) .
> The key point is to traverse every child of `CXXOperatorCallExpr` *except* the operator.  E.g. for the AST node below, we don't want to traverse its first child which is the operator.
> 
> ```
> -CXXOperatorCallExpr 0x5591a97f5280 <col:9, col:27> 'int' '+'
>     |-ImplicitCastExpr 0x5591a97f5268 <col:18> 'int (*)(string, string)' <FunctionToPointerDecay>
>     | `-DeclRefExpr 0x5591a97f51e8 <col:18> 'int (string, string)' lvalue Function 0x5591a97f4878 'operator+' 'int (string, string)'
>     |-CXXTemporaryObjectExpr 0x5591a97f5068 <col:9, col:16> 'string':'string' 'void () noexcept' zeroing
>     `-CXXTemporaryObjectExpr 0x5591a97f51b8 <col:20, col:27>
> ```
> 
> I added the `WalkUpFromCXXOperatorCallExpr` back (I think it doesn't have any effect on the current code because we don't do anything on the `CXXOperatorCallExpr` hierarchy classes).
Yeah, sorry I was confused and thought we handled function refs in CallExpr instead of DeclRefExpr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140551