Page MenuHomePhabricator

Fix diagnostic for addr spaces in reference binding
Needs ReviewPublic

Authored by Anastasia on Feb 11 2019, 9:05 AM.

Details

Reviewers
rjmccall
Summary

Extend reference binding behavior to account for address spaces. This change also fixes the diagnostic wording (for qualifier mismatch in reference binding) and simplifies it by using Qualifiers print method.

Diff Detail

Event Timeline

Anastasia created this revision.Feb 11 2019, 9:05 AM
rjmccall added inline comments.Feb 12 2019, 2:01 PM
include/clang/Basic/DiagnosticSemaKinds.td
1831

We say that references are bound to objects, not that objects are bound to references.

lib/Sema/SemaInit.cpp
4628

I think the comment should go after the quote in the main comment above, i.e. something like "For address spaces, we interpret this to mean..."

Also, this clause is part of the parenthesized group and should be indented consistently.

So static_cast permits conversions from AS1 to AS2 where that conversion is implicitly allowed, and the new addrspace_cast would permit conversions from AS1 to AS2 where it is explicitly allowed. That seems like it fits in rather well with the idea in D57464 regarding support for specifying permitted AS conversions in target.

How about nested pointers, such as __X int * * -> __Y int * * or __X int * __Y * -> int * __Y *? static_cast has some ruleset for how to deal with qualifiers in nested pointers, I think, but I'm not sure how the rules for ASes should be here.

There's also C-style casts. Will addrspace_cast be added to the C-style cast rules somewhere?

(I should probably have commented on the original RFC)

So static_cast permits conversions from AS1 to AS2 where that conversion is implicitly allowed, and the new addrspace_cast would permit conversions from AS1 to AS2 where it is explicitly allowed. That seems like it fits in rather well with the idea in D57464 regarding support for specifying permitted AS conversions in target.

How about nested pointers, such as __X int * * -> __Y int * * or __X int * __Y * -> int * __Y *? static_cast has some ruleset for how to deal with qualifiers in nested pointers, I think, but I'm not sure how the rules for ASes should be here.

We had discussion related to this with John earlier. And I documented it in this bug: https://bugs.llvm.org/show_bug.cgi?id=39674

There's also C-style casts. Will addrspace_cast be added to the C-style cast rules somewhere?

(I should probably have commented on the original RFC)

Yes, exactly. I am working on a patch now that separates the address space casting out into a function to be used in C-style cast only as a first step (because I would like to disallow accidental unsafe address space conversions in reinterpret_cast). Then we would just need to add parsing of a new cast operator and mapping into the new function.

  • Changed the diagnostic for binding reference and combined with existing similar one. That affected more tests however.
  • Changed comment explaining address space behavior in the reference initialization.
  • Reformatted.

We had discussion related to this with John earlier. And I documented it in this bug: https://bugs.llvm.org/show_bug.cgi?id=39674

I think we observed something similar to this downstream in C, and I might have a patch(es) that fixes it.

Yes, exactly. I am working on a patch now that separates the address space casting out into a function to be used in C-style cast only as a first step (because I would like to disallow accidental unsafe address space conversions in reinterpret_cast). Then we would just need to add parsing of a new cast operator and mapping into the new function.

Okay, sounds good!

Btw, I have changed the diagnostic wording... does this change make sense now?

Anastasia updated this revision to Diff 189306.Mar 5 2019, 5:07 AM
Anastasia retitled this revision from Fix diagnostic for addr spaces in static_cast to Fix diagnostic for addr spaces in reference binding.
Anastasia edited the summary of this revision. (Show Details)
  • Fixed incorrect diagnostic in address space mismatch case when binding reference to a temporary value of a constant with incompatible type. Added missing test case.
ebevhan added inline comments.Mar 5 2019, 6:29 AM
test/SemaOpenCLCXX/address-space-references.cl
8

Is there a reason the declaration is commented out? If it's just a problem with the resolution, wouldn't it still be possible to have the declaration here anyway?

Anastasia updated this revision to Diff 189351.Mar 5 2019, 10:15 AM
Anastasia marked an inline comment as done.

Un-commented the line in the test

Anastasia added inline comments.Mar 5 2019, 10:17 AM
test/SemaOpenCLCXX/address-space-references.cl
8

Hmm... actually not at all! I commented it to show that this could work better... but FIXME is good enough to indicate this!

@rjmccall or @ebevhan do you have any more feedback for this patch?

Btw, it has now dependency with https://reviews.llvm.org/D61318