[PATCH] D84226: [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14365
+           "Should only occur in error-recovery path.");
+    if (BinaryOperator::isCompoundAssignmentOp(Opc))
+      return CompoundAssignOperator::Create(
----------------
sammccall wrote:
> isAssignmentOp instead? including = itself
Simple assignment `=` doesn't belong to `CompoundAssignOperator`, it should be `BinaryOperator`.

Added the handling logic in the switch-case below.


================
Comment at: clang/test/AST/ast-dump-recovery.c:45
   int* ptr;
-  // FIXME: the top-level expr should be a binary operator.
-  // CHECK:      ImplicitCastExpr {{.*}} contains-errors <LValueToRValue>
-  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors lvalue
-  // CHECK-NEXT:   |-DeclRefExpr {{.*}} 'ptr' 'int *'
-  // CHECK-NEXT:   `-RecoveryExpr {{.*}}
-  // CHECK-NEXT:     `-DeclRefExpr {{.*}} 'some_func'
+  // CHECK:     BinaryOperator {{.*}} contains-errors '='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'ptr' 'int *'
----------------
sammccall wrote:
> `'int *' lvalue contains-errors '='`
oh, this spots a bug in our code -- unlike C++, it should not be a lvalue.

>From  C [6.15.16] p3:
> An assignment expression has the value of the left operand after the assignment, but is not an lvalue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84226