This is an archive of the discontinued LLVM Phabricator instance.

PR27037: Use correct CVR qualifier on an upcast on method pointer call
ClosedPublic

Authored by tzik on Jun 3 2017, 10:32 PM.

Details

Summary

Clang currently ignores a const qualifier of a method receiver when the method is invoked through
a method pointer, and the invocation implies an implicit cast to the parent class, which hurts the
const correctness. This patch fixes it by restoring the qualifier on the parent class from the child class.

Diff Detail

Repository
rL LLVM

Event Timeline

tzik created this revision.Jun 3 2017, 10:32 PM
tzik retitled this revision from Use correct CVR qualifier on an upcast on method pointer call to PR27037: Use correct CVR qualifier on an upcast on method pointer call.Jun 3 2017, 10:53 PM
tzik edited the summary of this revision. (Show Details)
tzik edited the summary of this revision. (Show Details)Jun 4 2017, 8:31 PM
tzik added a subscriber: cfe-commits.
tzik added a reviewer: rsmith.Jun 5 2017, 10:20 AM
tzik added a comment.Jun 5 2017, 10:24 AM

Hi, Richard.
Could you PTAL to this?

rsmith added inline comments.Jun 5 2017, 3:17 PM
lib/Sema/SemaExprCXX.cpp
5108 ↗(On Diff #101346)

In the "indirect" case, the cv-qualifiers should be taken from the pointee type of the LHS and applied to the pointee type of UseType. I believe this patch will not be enough to cause us to reject the indirect version of your testcase:

((&b)->*&B::f)();  // expected-error{{drops 'const' qualifier}}

Moreover, we should be preserving all qualifiers, not just cvr-qualifiers; for example, this should also preserve the address space.

tzik updated this revision to Diff 101536.Jun 6 2017, 3:59 AM

Cover indirect case and non-CVR qualifiers

tzik marked an inline comment as done.Jun 6 2017, 4:02 AM
tzik added inline comments.
lib/Sema/SemaExprCXX.cpp
5108 ↗(On Diff #101346)

Make sense. OK, I updated the CL to cover that cases.
Could you take another look?

rsmith accepted this revision.Jun 6 2017, 1:03 PM

Looks good to me, thanks! Do you need someone to commit this for you?

This revision is now accepted and ready to land.Jun 6 2017, 1:03 PM
tzik marked an inline comment as done.Jun 6 2017, 6:10 PM

Looks good to me, thanks! Do you need someone to commit this for you?

Yes, could you commit this?

This revision was automatically updated to reflect the committed changes.