[cfe-commits] r67059 - in /cfe/trunk: include/clang/AST/ include/clang/Basic/ lib/AST/ lib/CodeGen/ lib/Sema/ test/Parser/ test/SemaCXX/

[Douglas Gregor]

Hi Sebastian,

Sorry about the delay... comments follow.

On Mar 22, 2009, at 2:48 PM, Sebastian Redl wrote:

> Hi Doug,
>
> Thanks for the review. Some comments below.
>
> Douglas Gregor wrote:
>> On Mar 16, 2009, at 4:22 PM, Sebastian Redl wrote:
>>
>>> Modified: cfe/trunk/lib/Sema/SemaNamedCast.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaNamedCast.cpp?rev=67059&r1=67058&r2=67059&view=diff
>>>
>>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>>
>>> --- cfe/trunk/lib/Sema/SemaNamedCast.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaNamedCast.cpp Mon Mar 16 18:22:08 2009
>>> @@ -115,9 +115,10 @@
>>>
>>>  DestType = Self.Context.getCanonicalType(DestType);
>>>  QualType SrcType = SrcExpr->getType();
>>> -  if (const ReferenceType *DestTypeTmp =
>>> DestType->getAsReferenceType()) {
>>> +  if (const LValueReferenceType *DestTypeTmp =
>>> +        DestType->getAsLValueReferenceType()) {
>>>    if (SrcExpr->isLvalue(Self.Context) != Expr::LV_Valid) {
>>> -      // Cannot cast non-lvalue to reference type.
>>> +      // Cannot cast non-lvalue to lvalue reference type.
>>>      Self.Diag(OpRange.getBegin(), diag::err_bad_cxx_cast_rvalue)
>>>        << "const_cast" << OrigDestType << SrcExpr->getSourceRange();
>>>      return;
>>> @@ -141,6 +142,8 @@
>>>  if (!DestType->isPointerType() && !DestType->isMemberPointerType 
>>> ()) {
>>>    // Cannot cast to non-pointer, non-reference type. Note that, if
>>> DestType
>>>    // was a reference type, we converted it to a pointer above.
>>> +    // The status of rvalue references isn't entirely clear, but it
>>> looks like
>>> +    // conversion to them is simply invalid.
>>>    // C++ 5.2.11p3: For two pointer types [...]
>>>    Self.Diag(OpRange.getBegin(), diag::err_bad_const_cast_dest)
>>>      << OrigDestType << DestRange;
>>
>> Did you see [expr.const.cast]p4? It specifies that one can explicitly
>> convert to an rvalue of type T2 using the cast const_cast<T2&&>. So,
>> at the very least, we have to set DestType and SrcType appropriately
>> when we're converting to an rvalue reference.
> That feels wrong. It makes sense when "lvalue T"->"T&&" is implicit,
> that the const_cast is allowed. However, now that such a conversion
> requires a static_cast, I don't think const_cast<T&&>(lvalue) should  
> be
> valid. const_cast is for cv-qualifier modifications, and
> lvalue->rvalue-ref is static_cast's job.
> N2844 contains no wording to prohibit it, but I think it should.
> Specifically, in [expr.const.cast]p4 this sentence:
> "Similarly, for two effective object types T1 and T2, an expression of
> type T1 can be explicitly converted to an rvalue of type T2 using the
> cast const_cast<T2&&> if a pointer to T1 can be explicitly converted  
> to
> the type "pointer to T2" using a const_cast."
> should be amended to say:
> "... an rvalue expression of type T1 can be ..."
> There are probably similar things in the other casts.

Hmmm, interesting. My feeling here is that it's fine to explicitly  
treat an lvalue as an rvalue. It's only the implicit binding of rvalue  
references to lvalues that really cause problems.

Anyway, Clang has to interpret the working paper as written. If you  
feel that the specification of this feature is wrong, please bring it  
to the attention of the committee.

>>> @@ -4144,7 +4153,7 @@
>>>    // FIXME: Represent the user-defined conversion in the AST!
>>>    ImpCastExprToType(Object,
>>>                      Conv->getConversionType().getNonReferenceType 
>>> (),
>>> -                      Conv->getConversionType()->isReferenceType 
>>> ());
>>> +
>>> Conv->getConversionType()->isLValueReferenceType());
>>>    return ActOnCallExpr(S, ExprArg(*this, Object), LParenLoc,
>>>                         MultiExprArg(*this, (ExprTy**)Args,  
>>> NumArgs),
>>>                         CommaLocs, RParenLoc).release();
>>
>> It looks like you haven't implemented the implicit conversion ranking
>> logic for rvalue reference bindings. which would go near the end of
>> Sema::CompareStandardConversionSequences. This logic should be able  
>> to
>> choose appropriately between binding to "const X&" and binding to
>> "X&&" for rvalues. I've checked in a commented-out, FIXME'd test case
>> in rval-references.cpp that's currently not working.
> If I understand it correctly, X&& is *always* preferred, now that rval
> refs cannot bind to lvalues. Correct?

Yes.

>>
>> I accidentally ran the rval-references test without -std=c++0x, and
>> the error messages are a bit scary. I suggest that we parse the  
>> rvalue
>> reference regardless of C++ dialect but give a "rvalue references are
>> a C++0x extension" warning in C++03.
>>
> Just a warning? Having the parser recognize the thing in C++03 is a  
> good
> thing, but I don't know about actually allowing it.

Well, yeah, it should be an error.

>> We definitely need tests for static_casting to an rvalue reference  
>> type.
> We do. I'll add them when I add support for the casting. But I'll add
> them to the rvalue reference test, not the static_cast test. That one
> should stay C++03.

Okay.

>> To actually test this, we'll need some way to detect whether we're
>> getting the right constructors... since we don't have access control
>> or code-generation yet, the easiest way forward might be to put in  
>> the
>> syntax for ' = delete'. Most of the functionality is already there.
> I'll look at it.

Thanks!

	- Doug