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

[Sebastian Redl]

On Mon, 16 Mar 2009 17:02:57 -0700, Douglas Gregor <dgregor at apple.com>
wrote:
> On Mar 16, 2009, at 4:22 PM, Sebastian Redl wrote:
> 
>> Author: cornedbee
>> Date: Mon Mar 16 18:22:08 2009
>> New Revision: 67059
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=67059&view=rev
>> Log:
>> Almost complete implementation of rvalue references. One bug, and a  
>> few unclear areas. Maybe Doug can shed some light on some of the  
>> fixmes.
> 
> An excellent trade, indeed!
> 
>>   // FIXME: C++ shouldn't be going through here!  The rules are  
>> different
>>   // enough that they should be handled separately.
>> +  // FIXME: Merging of lvalue and rvalue references is incorrect. C+ 
>> + *really*
>> +  // shouldn't be going through here!
> 
> I wonder if C++ is actually going through here?

I'm pretty sure it doesn't. There's an assertion further down to that
effect, but after some early exits.
We could move it up and see what happens.

>>       case '&':
>> -        Type = Context.getReferenceType(Type);
>> +        Type = Context.getLValueReferenceType(Type);
>>         break;
>> +      // FIXME: There's no way to have a built-in with an rvalue  
>> ref arg.
> 
> Right, and that's a good thing (IMHO). I'd just remove this FIXME.
> 

OK.

>> +  // FIXME: Does GCC differ between lvalue and rvalue references  
>> here?
>>   enum gcc_type_class {
>>     no_type_class = -1,
>>     void_type_class, integer_type_class, char_type_class,
> 
> Oh, interesting. I don't recall this being part of the rvalue- 
> references implementation that went into GCC, so I suspect GCC does  
> not differentiate. We can check it with GCC >= 4.3.
> 

Yes, should be easy enough.

>> +    // Rvalue references cannot bind to lvalues (N2812).
>> +    // FIXME: This part of rvalue references is still in flux.  
>> Revisit later.
>> +    if (isRValRef) {
>> +      if (!ICS)
>> +        Diag(Init->getSourceRange().getBegin(),  
>> diag::err_lvalue_to_rvalue_ref)
>> +          << Init->getSourceRange();
>> +      return true;
>> +    }
>> +
> 
> This isn't in flux any more; earlier this month, in Summit, NJ, the C+ 
> + committee accepted the proposed wording that bans binding rvalue  
> references to lvalues. The final document is N2844, which will be in  
> the post-Summit mailing (available in ~2 weeks); I've attached a copy  
> of N2844.

Excellent.

>> +  // FIXME: C++0x [except.handle] names the handler as cv T or cv  
>> T&, i.e.
>> +  // rvalue references aren't there. Oversight or intentional?
> 
> Intentional. N2844 explicitly bans the use of rvalue references in the  
> handler.
> 

OK.

>>   case ICK_Qualification:
>> +    // FIXME: Not sure about lvalue vs rvalue here in the presence of
>> +    // rvalue references.
>>     ImpCastExprToType(From, ToType.getNonReferenceType(),
>> -                      ToType->isReferenceType());
>> +                      ToType->isLValueReferenceType());
>>     break;
> 
> This looks right to me. If we're implicitly converting to an rvalue- 
> reference type, then the implicit conversion produces an rvalue.
> 

OK.

>> +  if (LValueRef) {
>> +    if (const RValueReferenceType *R = T->getAsRValueReferenceType 
>> ()) {
>> +      // FIXME: Find the C++0x reference for reference collapsing.
>> +      // In reference collapsing, lvalue refs win over rvalue refs.
>> +      return Context.getLValueReferenceType(R->getPointeeType()).
>> +               getQualifiedType(Quals);
>> +    }
>> +  }
> 
> IIRC, in the reference-collapsing case we drop the qualifiers applied  
> to the outer reference.
> 

So, drop the getQualifiedType()?

Thanks for the review.
Sebastian