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

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked an inline comment as done.
steveire added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5346
+    AST_POLYMORPHIC_SUPPORTED_TYPES(BinaryOperator, CXXOperatorCallExpr,
+                                    CXXRewrittenBinaryOperator)) {
   return Node.isAssignmentOp();
----------------
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`).


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:260
+
+    auto DCF = Node->getDecomposedForm();
+    if (!match(*DCF.LHS) || !match(*DCF.RHS))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94130