[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 closed this revision. avt77 added a comment. Fixed by 324721. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
RKSimon added a comment. @avt77 Close this now that https://reviews.llvm.org/rL324721 has landed? https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 added a comment. Committed revision 324721. BTW, could you review https://reviews.llvm.org/D42728: it's rather similar to this one and rather small as well. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added a comment. Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM. Comment at: lib/Sema/SemaExpr.cpp:4402 +Qualifiers MemberQuals = +Context.getCanonicalType(ResultType).getQualifiers(); +Qualifiers Combined = BaseQuals + MemberQuals; You shouldn't have to get the canonical type here. Comment at: test/Sema/assign.c:17 + b[4] = 1; // expected-error {{cannot assign to variable 'b' with const-qualified type 'const arr' (aka 'int const[10]')}} + b2[4] = 1;// expected-error {{cannot assign to variable 'b2' with const-qualified type 'const int [10]'}} } These are nice improvements, thanks. There's still room for getting ever better, but this is already nice progress. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 updated this revision to Diff 133404. avt77 added a comment. I propagated qualifiers accordingly to rjmccall's suggestion. But I changed the diagnostic: now it's more realistic from my point of view. https://reviews.llvm.org/D42530 Files: lib/Sema/SemaExpr.cpp lib/Sema/SemaExprMember.cpp test/Sema/assign.c test/Sema/typedef-retain.c test/SemaCXX/err_typecheck_assign_const.cpp Index: test/SemaCXX/err_typecheck_assign_const.cpp === --- test/SemaCXX/err_typecheck_assign_const.cpp +++ test/SemaCXX/err_typecheck_assign_const.cpp @@ -129,3 +129,23 @@ Func &bar(); bar()() = 0; // expected-error {{read-only variable is not assignable}} } + +typedef float float4 __attribute__((ext_vector_type(4))); +struct OhNo { + float4 v; + void AssignMe() const { v.x = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo::AssignMe' is declared const here}} +}; + +typedef float float4_2 __attribute__((__vector_size__(16))); +struct OhNo2 { + float4_2 v; + void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo2::AssignMe' is declared const here}} +}; + +struct OhNo3 { + float v[4]; + void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo3::AssignMe' is declared const here}} +}; Index: test/Sema/typedef-retain.c === --- test/Sema/typedef-retain.c +++ test/Sema/typedef-retain.c @@ -16,8 +16,8 @@ typedef int a[5]; void test3() { typedef const a b; - b r; - r[0]=10; // expected-error {{read-only variable is not assignable}} + b r; // expected-note {{variable 'r' declared const here}} + r[0] = 10; // expected-error {{cannot assign to variable 'r' with const-qualified type 'b' (aka 'int const[5]')}} } int test4(const a y) { Index: test/Sema/assign.c === --- test/Sema/assign.c +++ test/Sema/assign.c @@ -11,10 +11,10 @@ typedef int arr[10]; void test3() { - const arr b; - const int b2[10]; - b[4] = 1; // expected-error {{read-only variable is not assignable}} - b2[4] = 1; // expected-error {{read-only variable is not assignable}} + const arr b; // expected-note {{variable 'b' declared const here}} + const int b2[10]; // expected-note {{variable 'b2' declared const here}} + b[4] = 1; // expected-error {{cannot assign to variable 'b' with const-qualified type 'const arr' (aka 'int const[10]')}} + b2[4] = 1;// expected-error {{cannot assign to variable 'b2' with const-qualified type 'const int [10]'}} } typedef struct I { Index: lib/Sema/SemaExprMember.cpp === --- lib/Sema/SemaExprMember.cpp +++ lib/Sema/SemaExprMember.cpp @@ -1623,10 +1623,14 @@ else VK = BaseExpr.get()->getValueKind(); } + QualType ret = CheckExtVectorComponent(S, BaseType, VK, OpLoc, Member, MemberLoc); if (ret.isNull()) return ExprError(); +Qualifiers BaseQ = +S.Context.getCanonicalType(BaseExpr.get()->getType()).getQualifiers(); +ret = S.Context.getQualifiedType(ret, BaseQ); return new (S.Context) ExtVectorElementExpr(ret, VK, BaseExpr.get(), *Member, MemberLoc); Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -4395,8 +4395,14 @@ if (VK != VK_RValue) OK = OK_VectorComponent; -// FIXME: need to deal with const... ResultType = VTy->getElementType(); +QualType BaseType = BaseExpr->getType(); +Qualifiers BaseQuals = BaseType.getQualifiers(); +Qualifiers MemberQuals = +Context.getCanonicalType(ResultType).getQualifiers(); +Qualifiers Combined = BaseQuals + MemberQuals; +if (Combined != MemberQuals) + ResultType = Context.getQualifiedType(ResultType, Combined); } else if (LHSTy->isArrayType()) { // If we see an array that wasn't promoted by // DefaultFunctionArrayLvalueConversion, it must be an array that @@ -10434,8 +10440,16 @@ // Static fields do not inherit constness from parents. break; } - break; -} // End MemberExpr + break; // End MemberExpr +} else if (const ArraySubscriptExpr *ASE = + dyn_cast(E)) { + E = ASE->getBase()->IgnoreParenImpCasts(); + continue; +} else if (const ExtVectorElementExpr *EVE = +
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added a comment. No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 updated this revision to Diff 132607. avt77 added a comment. I re-implemented the patch. Now it works properly with const methods in CXX classes. https://reviews.llvm.org/D42530 Files: lib/AST/ExprClassification.cpp lib/Sema/SemaExpr.cpp test/Sema/assign.c test/Sema/typedef-retain.c test/SemaCXX/anonymous-union.cpp test/SemaCXX/err_typecheck_assign_const.cpp Index: test/SemaCXX/err_typecheck_assign_const.cpp === --- test/SemaCXX/err_typecheck_assign_const.cpp +++ test/SemaCXX/err_typecheck_assign_const.cpp @@ -129,3 +129,23 @@ Func &bar(); bar()() = 0; // expected-error {{read-only variable is not assignable}} } + +typedef float float4 __attribute__((ext_vector_type(4))); +struct OhNo { + float4 v; + void AssignMe() const { v.x = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo::AssignMe' is declared const here}} +}; + +typedef float float4_2 __attribute__((__vector_size__(16))); +struct OhNo2 { + float4_2 v; + void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo2::AssignMe' is declared const here}} +}; + +struct OhNo3 { + float v[4]; + void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \ +expected-note {{member function 'OhNo3::AssignMe' is declared const here}} +}; Index: test/SemaCXX/anonymous-union.cpp === --- test/SemaCXX/anonymous-union.cpp +++ test/SemaCXX/anonymous-union.cpp @@ -40,7 +40,8 @@ } void X::test_unqual_references_const() const { // expected-note 2{{member function 'X::test_unqual_references_const' is declared const here}} - d = 0.0; + // TODO: it seems we should not see any error here becuase 'd' is mutable + d = 0.0; // expected-error {{read-only variable is not assignable}} f2 = 0; // expected-error{{cannot assign to non-static data member within const member function 'test_unqual_references_const'}} a = 0; // expected-error{{cannot assign to non-static data member within const member function 'test_unqual_references_const'}} } Index: test/Sema/typedef-retain.c === --- test/Sema/typedef-retain.c +++ test/Sema/typedef-retain.c @@ -16,8 +16,8 @@ typedef int a[5]; void test3() { typedef const a b; - b r; - r[0]=10; // expected-error {{read-only variable is not assignable}} + b r; // expected-note {{variable 'r' declared const here}} + r[0]=10; // expected-error {{cannot assign to variable 'r' with const-qualified type 'b' (aka 'int const[5]')}} } int test4(const a y) { Index: test/Sema/assign.c === --- test/Sema/assign.c +++ test/Sema/assign.c @@ -11,10 +11,10 @@ typedef int arr[10]; void test3() { - const arr b; - const int b2[10]; - b[4] = 1; // expected-error {{read-only variable is not assignable}} - b2[4] = 1; // expected-error {{read-only variable is not assignable}} + const arr b; // expected-note {{variable 'b' declared const here}} + const int b2[10]; // expected-note {{variable 'b2' declared const here}} + b[4] = 1; // expected-error {{cannot assign to variable 'b' with const-qualified type 'const arr' (aka 'int const[10]')}} + b2[4] = 1; // expected-error {{cannot assign to variable 'b2' with const-qualified type 'const int [10]'}} } typedef struct I { Index: lib/Sema/SemaExpr.cpp === --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -10321,7 +10321,7 @@ /// 'const' due to being captured within a block? enum NonConstCaptureKind { NCCK_None, NCCK_Block, NCCK_Lambda }; static NonConstCaptureKind isReferenceToNonConstCapture(Sema &S, Expr *E) { - assert(E->isLValue() && E->getType().isConstQualified()); + assert(E->isLValue()); E = E->IgnoreParens(); // Must be a reference to a declaration from an enclosing scope. @@ -10401,10 +10401,8 @@ const ValueDecl *VD = ME->getMemberDecl(); if (const FieldDecl *Field = dyn_cast(VD)) { // Mutable fields can be modified even if the class is const. -if (Field->isMutable()) { - assert(DiagnosticEmitted && "Expected diagnostic not emitted."); +if (Field->isMutable()) break; -} if (!IsTypeModifiable(Field->getType(), IsDereference)) { if (!DiagnosticEmitted) { @@ -10434,8 +10432,16 @@ // Static fields do not inherit constness from parents. break; } - break; -} // End MemberExpr +
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added a comment. If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in the code. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 added a comment. In https://reviews.llvm.org/D42530#995227, @rjmccall wrote: > That's still just const-propagation. The const qualifier on the pointee type > of this should propagate to the l-value resulting from the member access, and > the vector-specific projection logic should continue to propagate const to > the type of the element l-value. I'm afraid it's wrong because from my point of view Clang generates wrong message for test OhNo3 (see err_typecheck_assign_const.cpp). It should not be "read-only variable is not assignable" because v[4] is not read-only variable. Any other non-const method is able to change this variable w/o any problems. The the same is for OhNo and OhNo2. BTW, gcc generates error: assignment of read-only location ‘*(const float*)(&((const OhNo2*)this)->OhNo2::v)’ and that's OK because we try to change the value through const-pointer-to-const. Am I right? https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added a comment. That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 added a comment. In fact we have here another problem: it is not an attempt to assign to const variable but it is an attempt to use the field assingment inside const method: I'm working on it. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + avt77 wrote: > rjmccall wrote: > > Is there a reason why the places that compute the type of these l-value > > expressions don't propagate qualiifers? This hardly seems restricted to > > 'const'. > Do you mean ExtVectorElementExpr/ArraySubscriptExpr should retrun > isConstQualified() == true by themselves (and other qualifiers as well)? In the sense that Sema should create them with a type that is const-qualified when the underlying vector l-value is const-qualified, yes. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
avt77 added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + rjmccall wrote: > Is there a reason why the places that compute the type of these l-value > expressions don't propagate qualiifers? This hardly seems restricted to > 'const'. Do you mean ExtVectorElementExpr/ArraySubscriptExpr should retrun isConstQualified() == true by themselves (and other qualifiers as well)? https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method
rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + Is there a reason why the places that compute the type of these l-value expressions don't propagate qualiifers? This hardly seems restricted to 'const'. https://reviews.llvm.org/D42530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits