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

[Sebastian Redl]

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.
>> @@ -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?
>
> It turns out that this behavior works fine, but since virr1 was marked
> invalid (due to the missing initialization), we weren't performing the
> type-check. Yay!
>
Doh!
>
> 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.
> 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.

> 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.

Sebastian