[PATCH] D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/AST/ast-dump-recovery.c:75 + + // conditional operator + float f; add to comment: (comparison is invalid) Comment at: clang/test/AST/ast-dump-recovery.c:83 + // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue + ptr > f ? ptr : f; } again - parens Comment at: clang/test/Sema/error-dependence.c:18 + // type is required" is not emitted. + ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}} +} hokein wrote: > sammccall wrote: > > nit: parens would help me understand here :-) > > > > I don't find this example compelling because we should know that "ptr > f" > > is a boolean. > > > > Can we just have `undefined ? ptr : f`, expecting the only diag to be the > > undeclared ident? > > I don't find this example compelling because we should know that "ptr > f" > > is a boolean. > > this is a reasonable impression, but in this case, there is no > binary-operator `>` -- operands `ptr`, `f` have different types, and invalid > for binary operator, instead we build a recovery-expr. > > so the AST looks like (added in the dump-recovery.c test as well) > > ``` > ConditionalOperator> '' contains-errors > | |-RecoveryExpr '' contains-errors lvalue > | | |-DeclRefExpr 'int *' lvalue Var 0x8fdb620 'ptr' 'int *' > | | `-DeclRefExpr 'float' lvalue Var 0x8ffd388 'f' 'float' > | |-DeclRefExpr 'int *' lvalue Var 0x8fdb620 'ptr' 'int *' > | `-DeclRefExpr 'float' lvalue Var 0x8ffd388 'f' 'float' > ``` > > > Can we just have undefined ? ptr : f, expecting the only diag to be the > > undeclared ident? > > no unfortunately. the whole statement is being dropped (we don't preserve > this in C/C++). `undefined() ? ptr : f` then? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84322/new/ https://reviews.llvm.org/D84322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic
hokein added inline comments. Comment at: clang/test/Sema/error-dependence.c:18 + // type is required" is not emitted. + ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}} +} sammccall wrote: > nit: parens would help me understand here :-) > > I don't find this example compelling because we should know that "ptr > f" is > a boolean. > > Can we just have `undefined ? ptr : f`, expecting the only diag to be the > undeclared ident? > I don't find this example compelling because we should know that "ptr > f" is > a boolean. this is a reasonable impression, but in this case, there is no binary-operator `>` -- operands `ptr`, `f` have different types, and invalid for binary operator, instead we build a recovery-expr. so the AST looks like (added in the dump-recovery.c test as well) ``` ConditionalOperator> '' contains-errors | |-RecoveryExpr '' contains-errors lvalue | | |-DeclRefExpr 'int *' lvalue Var 0x8fdb620 'ptr' 'int *' | | `-DeclRefExpr 'float' lvalue Var 0x8ffd388 'f' 'float' | |-DeclRefExpr 'int *' lvalue Var 0x8fdb620 'ptr' 'int *' | `-DeclRefExpr 'float' lvalue Var 0x8ffd388 'f' 'float' ``` > Can we just have undefined ? ptr : f, expecting the only diag to be the > undeclared ident? no unfortunately. the whole statement is being dropped (we don't preserve this in C/C++). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84322/new/ https://reviews.llvm.org/D84322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic
hokein updated this revision to Diff 296374. hokein added a comment. rebase and add ast-dump test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84322/new/ https://reviews.llvm.org/D84322 Files: clang/lib/Sema/SemaExpr.cpp clang/test/AST/ast-dump-recovery.c clang/test/Sema/error-dependence.c Index: clang/test/Sema/error-dependence.c === --- clang/test/Sema/error-dependence.c +++ clang/test/Sema/error-dependence.c @@ -7,3 +7,9 @@ // not emitted. s = call(); // expected-error {{too few arguments to function call}} } + +void test2(int* ptr, float f) { + // verify diagnostic "used type '' where arithmetic or pointer + // type is required" is not emitted. + ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}} +} Index: clang/test/AST/ast-dump-recovery.c === --- clang/test/AST/ast-dump-recovery.c +++ clang/test/AST/ast-dump-recovery.c @@ -71,4 +71,14 @@ // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func' // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 some_func(), 1; + + // conditional operator + float f; + // CHECK: ConditionalOperator {{.*}} '' contains-errors + // CHECK-NEXT: |-RecoveryExpr {{.*}} '' + // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue + // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue + // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue + // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue + ptr > f ? ptr : f; } Index: clang/lib/Sema/SemaExpr.cpp === --- clang/lib/Sema/SemaExpr.cpp +++ clang/lib/Sema/SemaExpr.cpp @@ -8067,6 +8067,16 @@ VK = VK_RValue; OK = OK_Ordinary; + if (Context.isDependenceAllowed() && + (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() || + RHS.get()->isTypeDependent())) { +assert(!getLangOpts().CPlusPlus); +assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() || + RHS.get()->containsErrors() && + "should only occur in error-recovery path."); +return Context.DependentTy; + } + // The OpenCL operator with a vector condition is sufficiently // different to merit its own checker. if ((getLangOpts().OpenCL && Cond.get()->getType()->isVectorType()) || Index: clang/test/Sema/error-dependence.c === --- clang/test/Sema/error-dependence.c +++ clang/test/Sema/error-dependence.c @@ -7,3 +7,9 @@ // not emitted. s = call(); // expected-error {{too few arguments to function call}} } + +void test2(int* ptr, float f) { + // verify diagnostic "used type '' where arithmetic or pointer + // type is required" is not emitted. + ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}} +} Index: clang/test/AST/ast-dump-recovery.c === --- clang/test/AST/ast-dump-recovery.c +++ clang/test/AST/ast-dump-recovery.c @@ -71,4 +71,14 @@ // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func' // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 some_func(), 1; + + // conditional operator + float f; + // CHECK: ConditionalOperator {{.*}} '' contains-errors + // CHECK-NEXT: |-RecoveryExpr {{.*}} '' + // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue + // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue + // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue + // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue + ptr > f ? ptr : f; } Index: clang/lib/Sema/SemaExpr.cpp === --- clang/lib/Sema/SemaExpr.cpp +++ clang/lib/Sema/SemaExpr.cpp @@ -8067,6 +8067,16 @@ VK = VK_RValue; OK = OK_Ordinary; + if (Context.isDependenceAllowed() && + (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() || + RHS.get()->isTypeDependent())) { +assert(!getLangOpts().CPlusPlus); +assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() || + RHS.get()->containsErrors() && + "should only occur in error-recovery path."); +return Context.DependentTy; + } + // The OpenCL operator with a vector condition is sufficiently // different to merit its own checker. if ((getLangOpts().OpenCL && Cond.get()->getType()->isVectorType()) || ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic
sammccall added inline comments. Comment at: clang/test/Sema/error-dependence.c:18 + // type is required" is not emitted. + ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}} +} nit: parens would help me understand here :-) I don't find this example compelling because we should know that "ptr > f" is a boolean. Can we just have `undefined ? ptr : f`, expecting the only diag to be the undeclared ident? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84322/new/ https://reviews.llvm.org/D84322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits