Clang permits assignment to vector/extvector elements in a const method
ClosedPublic

Authored by avt77 on Jan 25 2018, 5:13 AM.

Details

Summary

This patch should close Bug 35951.

Diff Detail

avt77 created this revision.Jan 25 2018, 5:13 AM
rjmccall added inline comments.Jan 25 2018, 11:19 AM
lib/AST/ExprClassification.cpp
652 ↗(On Diff #131422)

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'.

avt77 added inline comments.Jan 26 2018, 1:46 AM
lib/AST/ExprClassification.cpp
652 ↗(On Diff #131422)

Do you mean ExtVectorElementExpr/ArraySubscriptExpr should retrun isConstQualified() == true by themselves (and other qualifiers as well)?

rjmccall added inline comments.Jan 26 2018, 11:13 AM
lib/AST/ExprClassification.cpp
652 ↗(On Diff #131422)

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.

avt77 added a comment.Feb 1 2018, 9:16 AM

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.

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.

avt77 added a comment.Feb 2 2018, 12:21 AM

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?

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.

avt77 updated this revision to Diff 132607.Feb 2 2018, 8:45 AM

I re-implemented the patch. Now it works properly with const methods in CXX classes.

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.

avt77 updated this revision to Diff 133404.Feb 8 2018, 5:37 AM

I propagated qualifiers accordingly to rjmccall's suggestion. But I changed the diagnostic: now it's more realistic from my point of view.

Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM.

lib/Sema/SemaExpr.cpp
4402

You shouldn't have to get the canonical type here.

test/Sema/assign.c
17

These are nice improvements, thanks. There's still room for getting ever better, but this is already nice progress.

avt77 added a comment.Feb 9 2018, 1:36 AM

Committed revision 324721.
BTW, could you review D42728: it's rather similar to this one and rather small as well.

@avt77 Close this now that rL324721 has landed?

avt77 accepted this revision.Feb 12 2018, 4:36 AM
This revision is now accepted and ready to land.Feb 12 2018, 4:36 AM
avt77 closed this revision.Feb 12 2018, 4:37 AM

Fixed by 324721.