Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

Adam Strzelecki
John, I kindly ask for an explanation of following change introduced by you in f6a164819:

git diff f6a164819^! lib/Sema/SemaExpr.cpp

+    // We don't want to throw lvalue-to-rvalue casts on top of
+    // expressions of certain types in C++.
+    if (getLangOptions().CPlusPlus &&
+        (E->getType() == Context.OverloadTy ||
+         T->isDependentType() ||
+         T->isRecordType()))
+      return;

The problem with this change is that I explicitly request to use C style assignment (ignoring constructors) on POD type variable having `pod_assign` attribute, but condition above prevents proper cast to be emitted.

So far I have simply disabled, since I no longer have direct access to the Entity (VarDecl) to check for PODAssignAttrt, but I am not sure what are consequences of such step.

FYI:

https://github.com/ujhpc/clang/commit/61823c2bc5be78f793bdc8c11365d2bcb50455cc

+++ b/lib/Sema/SemaInit.cpp
@@ -4599,6 +4599,10 @@ void InitializationSequence::InitializeFrom(Sema &S,
       
   //     - If the destination type is a (possibly cv-qualified) class type:
   if (DestType->isRecordType()) {
+    if (Entity.getDecl()->hasAttr<PODAssignAttr>()) {
+      AddCAssignmentStep(DestType);
+      return;
+    }

Regards,
--
Adam
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

Richard Smith
On Sun, May 4, 2014 at 4:53 AM, Adam Strzelecki <[hidden email]> wrote:
John, I kindly ask for an explanation of following change introduced by you in f6a164819:

git diff f6a164819^! lib/Sema/SemaExpr.cpp

+    // We don't want to throw lvalue-to-rvalue casts on top of
+    // expressions of certain types in C++.
+    if (getLangOptions().CPlusPlus &&
+        (E->getType() == Context.OverloadTy ||
+         T->isDependentType() ||
+         T->isRecordType()))
+      return;

The problem with this change is that I explicitly request to use C style assignment (ignoring constructors) on POD type variable having `pod_assign` attribute, but condition above prevents proper cast to be emitted.

You're trying to shoehorn non-C++ semantics into C++, so you're going to have a bad time. The above check is appropriate for C++ (CK_LValueToRValue is not something that should ever happen to a record type in C++, because that's not what the lvalue-to-rvalue conversion does on record types).
 
So far I have simply disabled, since I no longer have direct access to the Entity (VarDecl) to check for PODAssignAttrt, but I am not sure what are consequences of such step.

FYI:

https://github.com/ujhpc/clang/commit/61823c2bc5be78f793bdc8c11365d2bcb50455cc

+++ b/lib/Sema/SemaInit.cpp
@@ -4599,6 +4599,10 @@ void InitializationSequence::InitializeFrom(Sema &S,

   //     - If the destination type is a (possibly cv-qualified) class type:
   if (DestType->isRecordType()) {
+    if (Entity.getDecl()->hasAttr<PODAssignAttr>()) {
+      AddCAssignmentStep(DestType);
+      return;
+    }

Regards,
--
Adam
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

John McCall
In reply to this post by Adam Strzelecki
On May 4, 2014, at 4:53 AM, Adam Strzelecki <[hidden email]> wrote:

> John, I kindly ask for an explanation of following change introduced by you in f6a164819:
>
> git diff f6a164819^! lib/Sema/SemaExpr.cpp
>
> +    // We don't want to throw lvalue-to-rvalue casts on top of
> +    // expressions of certain types in C++.
> +    if (getLangOptions().CPlusPlus &&
> +        (E->getType() == Context.OverloadTy ||
> +         T->isDependentType() ||
> +         T->isRecordType()))
> +      return;
>
> The problem with this change is that I explicitly request to use C style assignment (ignoring constructors) on POD type variable having `pod_assign` attribute, but condition above prevents proper cast to be emitted.


It is correct language behavior that, in C++, default l-value conversions — dropping cv-qualifiers and changing the value kind — don’t generally apply to expressions of class type.  We rely on this extensively through the compiler; in particular, we assume that non-POD record types will always be copied by an CXXOperatorCallExpr or CXXConstructExpr.  So removing this will cause assertions in a number of places, and if you simply disable all those assertions, you’ll massively break C++ semantics.

I don’t think you actually need to change this in Sema, though, unless it’s critical that you don’t trigger instantiation of copy constructors and/or assignment operators.  It should be fine to proceed through type-checking normally, maybe marking a few copy expressions as special (e.g. the same way that CXXConstructExprs are marked as elidable, you could alternatively mark them as implementable with memcpy) and then adding special-case logic to IR-generation.

You’re just planning to do this for your edification, right?  I'd have strong reservations about casually accepting work like this into trunk; changing type behavior like this has some pretty serious language design repercussions.  What does it mean, exactly, to use C-style assignment on a specific variable?  Does it only affect initializations of that variable, or does it also affect assignments to it, destruction of it, reads out of it, etc.?  Does your use case really require direct language support, and if so, is there a less invasive change that would let you do the rest in a library?

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

Alp Toker

On 05/05/2014 02:54, John McCall wrote:

> On May 4, 2014, at 4:53 AM, Adam Strzelecki <[hidden email]> wrote:
>> John, I kindly ask for an explanation of following change introduced by you in f6a164819:
>>
>> git diff f6a164819^! lib/Sema/SemaExpr.cpp
>>
>> +    // We don't want to throw lvalue-to-rvalue casts on top of
>> +    // expressions of certain types in C++.
>> +    if (getLangOptions().CPlusPlus &&
>> +        (E->getType() == Context.OverloadTy ||
>> +         T->isDependentType() ||
>> +         T->isRecordType()))
>> +      return;
>>
>> The problem with this change is that I explicitly request to use C style assignment (ignoring constructors) on POD type variable having `pod_assign` attribute, but condition above prevents proper cast to be emitted.
>
> It is correct language behavior that, in C++, default l-value conversions — dropping cv-qualifiers and changing the value kind — don’t generally apply to expressions of class type.  We rely on this extensively through the compiler; in particular, we assume that non-POD record types will always be copied by an CXXOperatorCallExpr or CXXConstructExpr.  So removing this will cause assertions in a number of places, and if you simply disable all those assertions, you’ll massively break C++ semantics.
>
> I don’t think you actually need to change this in Sema, though, unless it’s critical that you don’t trigger instantiation of copy constructors and/or assignment operators.  It should be fine to proceed through type-checking normally, maybe marking a few copy expressions as special (e.g. the same way that CXXConstructExprs are marked as elidable, you could alternatively mark them as implementable with memcpy) and then adding special-case logic to IR-generation.
>
> You’re just planning to do this for your edification, right?  I'd have strong reservations about casually accepting work like this into trunk; changing type behavior like this has some pretty serious language design repercussions.  What does it mean, exactly, to use C-style assignment on a specific variable?  Does it only affect initializations of that variable, or does it also affect assignments to it, destruction of it, reads out of it, etc.?  Does your use case really require direct language support, and if so, is there a less invasive change that would let you do the rest in a library?

For the record there's already an active thread on this where
alternatives have been suggested (Adam, can you keep the discussion in
one place so as not to duplicate effort?)

Top tip: If you want to see what a piece of code is for, try removing
that code and running the tests. The failing tests will indicate which
feature it was supporting without having to go to the author.

Alp.

--
http://www.nuanti.com
the browser experts

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

Adam Strzelecki
> For the record there's already an active thread on this where alternatives have been suggested (Adam, can you keep the discussion in one place so as not to duplicate effort?)

Apparently I did something wrong when I was trying to just change the subject but reference original thread. I am sorry for this duplication.

> Top tip: If you want to see what a piece of code is for, try removing that code and running the tests. The failing tests will indicate which feature it was supporting without having to go to the author.

Okay. Thanks, gonna try that first next time.

So let's move back to original thread.

Regards,
--
Adam
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: f6a164819 We don't want to throw lvalue-to-rvalue casts (...) of certain types in C++ (was: Allow implicit copy constructor (...))

Adam Strzelecki
In reply to this post by John McCall
> You’re just planning to do this for your edification, right?

Yes, that was the point.

Cheers,
--
Adam


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev