[PATCH] D94130: [ASTMatchers] Add support for CXXRewrittenBinaryOperator

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from the `auto` usage.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5346
+    AST_POLYMORPHIC_SUPPORTED_TYPES(BinaryOperator, CXXOperatorCallExpr,
+                                    CXXRewrittenBinaryOperator)) {
   return Node.isAssignmentOp();
----------------
steveire wrote:
> aaron.ballman wrote:
> > This change surprises me -- aren't rewritten binary operators *always* comparison operations? I don't know of a time when they'd ever be an assignment operator.
> Yes, which is why the new method in this patch on that class returns `true`.
> 
> This makes it possible to write
> 
> ```
> binaryOperation(isComparisonOperator())
> binaryOperation(isAssignmentOperator())
> ```
> 
> (possibly with `unless`).
Ohhh, I see what the intent is now. Thank you for clarifying!


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:260
+
+    auto DCF = Node->getDecomposedForm();
+    if (!match(*DCF.LHS) || !match(*DCF.RHS))
----------------
steveire wrote:
> aaron.ballman wrote:
> > Please spell out this use of `auto`.
> Please specify what to replace auto with when the replacement is a condition of acceptance.
> 
> Type noise makes code less readable, especially if the type contains `<`, `>` `,`, spaces or `::` or a combination of all of them. There is also ambiguity about what part of the type is a namespace, and what is an enclosing class etc. Very confusing,
> 
> In C++98 typedefs were often used to make that problem less bad. 
> 
> The C++11 solution is `auto`.
> 
> So, you require that an `auto` must be replaced with something less optimal, please be specific about what you require it replaced with. I don't know what you want here, but my guess is it is a long type which makes the code less readable.
> Please specify what to replace auto with when the replacement is a condition of acceptance.

If I knew that information, I would likely have not had to make the suggestion in the first place (or I would have suggested it as an edit). FWIW, I think it's a bit unreasonable to expect your code reviewers to hunt down the information you have at your fingertips as the code author (you presumably know what types your code edits are using). 

>From https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable:

"Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context."

There's nothing about this context that makes the type obvious to me. That said, I did the work and found the type should be spelled `CXXRewriteBinaryOperator::DecomposedForm`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94130