[PATCH 2/3] Implicit address space conversion on POD assign

[Richard Smith]

On Sun, May 18, 2014 at 5:19 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 19/05/2014 02:29, Adam Strzelecki wrote:
>
>> When assigning to POD without user defined copy constructor implicit
>> address
>> space conversion will be done if assigned value has same type but
>> different
>> address space and no matching constructor was found.
>>
>> This allows OpenCL address space conversion in C++ mode for POD types.
>> ---
>>   lib/Sema/SemaInit.cpp                     | 22 +++++++++++++++++++++-
>>   test/SemaCXX/address-space-initialize.cpp | 28
>> ++++++++++++++++++++++++++++
>>   2 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
>> index fe1f7c8..63ddd72 100644
>> --- a/lib/Sema/SemaInit.cpp
>> +++ b/lib/Sema/SemaInit.cpp
>> @@ -4606,9 +4606,28 @@ void InitializationSequence::InitializeFrom(Sema
>> &S,
>>       if (Kind.getKind() == InitializationKind::IK_Direct ||
>>           (Kind.getKind() == InitializationKind::IK_Copy &&
>>            (Context.hasSameUnqualifiedType(SourceType, DestType) ||
>> -          S.IsDerivedFrom(SourceType, DestType))))
>> +          S.IsDerivedFrom(SourceType, DestType)))) {
>> +
>>         TryConstructorInitialization(S, Entity, Kind, Args,
>>                                      Entity.getType(), *this);
>> +
>> +      // Relax rules and do implicit conversion if copy constructor
>> +      // initialization fails due address space mismatch and destination
>> class
>> +      // is POD without user defined copy constructor
>> +      if (Failed() && Failure == FK_ConstructorOverloadFailed &&
>> +          Kind.getKind() == InitializationKind::IK_Copy &&
>> +          SourceType.getAddressSpace() != DestType.getAddressSpace()) {
>> +        const RecordType *DestRecordType = DestType->getAs<RecordType>();
>> +        CXXRecordDecl *DestRecordDecl
>> +          = cast<CXXRecordDecl>(DestRecordType->getDecl());
>> +        if (DestRecordDecl->isPOD() &&
>> +            !DestRecordDecl->hasUserDeclaredCopyConstructor() &&
>> +            DestRecordDecl->hasCopyConstructorWithConstParam()) {
>> +          setSequenceKind(SequenceKind::NormalSequence);
>> +          goto ImplicitConversion;
>> +        }
>> +      }
>> +    }
>>
>
> Would your use case be satisfied with isCXX11PODType(), essentially
> RD->isTrivial() && RD->isStandardLayout()?
>
> If so (and if there's interest in going with this) it'd be preferable to
> define the language extension in those existing terms.
>

I think you're right that using POD here is a mistake, but I don't really
think that trivial and standard layout are the right things to check.
Instead, I think the rule should be something like: if you cannot
initialize an X from a Y, and dropping the address space from Y allows the
initialization to succeed in such a way that it only invokes trivial
copy/move constructors, then the initialization is permitted, and is
performed by first copying Y into X's address space, then calling the
trivial constructor (the latter copy should always be elided in practice).

Do we want an extension diagnostic here? As David Chisnall pointed out
> there are other possible interpretations even if this happens to be the one
> we already use in C mode.
>

Do we have some underlying standard of which this is an extension?


> Alp.
>
>
>
>        //     - Otherwise (i.e., for the remaining copy-initialization
>> cases),
>>       //       user-defined conversion sequences that can convert from
>> the source
>>       //       type to the destination type or (when a conversion
>> function is
>> @@ -4636,6 +4655,7 @@ void InitializationSequence::InitializeFrom(Sema
>> &S,
>>       return;
>>     }
>>   +ImplicitConversion:
>>     //    - Otherwise, the initial value of the object being initialized
>> is the
>>     //      (possibly converted) value of the initializer expression.
>> Standard
>>     //      conversions (Clause 4) will be used, if necessary, to convert
>> the
>> diff --git a/test/SemaCXX/address-space-initialize.cpp
>> b/test/SemaCXX/address-space-initialize.cpp
>> index 5091338..293a9cf 100644
>> --- a/test/SemaCXX/address-space-initialize.cpp
>> +++ b/test/SemaCXX/address-space-initialize.cpp
>> @@ -23,3 +23,31 @@ int* as_ptr = nocv_iarray; // expected-error{{cannot
>> initialize a variable of ty
>>   __attribute__((address_space(42))) int* __attribute__((address_space(42)))
>> ptr_in_same_addr_space = nocv_iarray;
>>   __attribute__((address_space(42))) int* __attribute__((address_space(999)))
>> ptr_in_different_addr_space = nocv_iarray;
>>   +
>> +struct A { virtual ~A() {} }; // \
>> +  expected-note 1 {{candidate constructor (the implicit copy
>> constructor)}} \
>> +  expected-note 1 {{candidate constructor (the implicit default
>> constructor)}}
>> +struct C { float x, y; };
>> +struct D : C { D(const D &i) {} }; // \
>> +  expected-note 1 {{candidate constructor not viable}}
>> +
>> +typedef A *A_ptr;
>> +typedef A __attribute__((address_space(1))) *A_ptr_1;
>> +typedef C *C_ptr;
>> +typedef C __attribute__((address_space(1))) *C_ptr_1;
>> +typedef D *D_ptr;
>> +typedef D __attribute__((address_space(1))) *D_ptr_1;
>> +
>> +void test_assignment(A_ptr ap, A_ptr_1 ap1,
>> +                     C_ptr cp, C_ptr_1 cp1,
>> +                     D_ptr dp, D_ptr_1 dp1) {
>> +  // No address space conversions
>> +  A a1 = ap[0];
>> +  C c1 = cp[0];
>> +  D d1 = dp[0];
>> +
>> +  // Implicit address space conversion allowed only for POD without user
>> defined copy constructor (C)
>> +  A a2 = ap1[0]; // expected-error{{no matching constructor for
>> initialization}}
>> +  C c2 = cp1[0];
>> +  D d2 = dp1[0]; // expected-error{{no matching constructor for
>> initialization}}
>> +}
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140518/25dfd623/attachment.html>