[cfe-commits] r77497 - in /cfe/trunk/lib/Sema: Sema.h SemaExpr.cpp

[Douglas Gregor]

On Jul 29, 2009, at 12:40 PM, Fariborz Jahanian wrote:

> Author: fjahanian
> Date: Wed Jul 29 14:40:11 2009
> New Revision: 77497
>
> URL: http://llvm.org/viewvc/llvm-project?rev=77497&view=rev
> Log:
> Check accessibility when converting object to the base
> class.

Thanks! Some more comments below.

> Modified:
>    cfe/trunk/lib/Sema/Sema.h
>    cfe/trunk/lib/Sema/SemaExpr.cpp
>
> Modified: cfe/trunk/lib/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.h?rev=77497&r1=77496&r2=77497&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Sema/Sema.h (original)
> +++ cfe/trunk/lib/Sema/Sema.h Wed Jul 29 14:40:11 2009
> @@ -691,7 +691,7 @@
>   ImplicitConversionSequence TryContextuallyConvertToBool(Expr *From);
>   bool PerformContextuallyConvertToBool(Expr *&From);
>
> -  void PerformObjectMemberConversion(Expr *&From, NamedDecl *Member);
> +  bool PerformObjectMemberConversion(Expr *&From, NamedDecl *Member);
>
>   /// OverloadingResult - Capture the result of performing overload
>   /// resolution.
>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=77497&r1=77496&r2=77497&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Wed Jul 29 14:40:11 2009
> @@ -1024,7 +1024,7 @@
>   return BuildDeclarationNameExpr(Loc, D, HasTrailingLParen, SS,  
> isAddressOfOperand);
> }
> /// \brief Cast member's object to its own class if necessary.
> -void
> +bool
> Sema::PerformObjectMemberConversion(Expr *&From, NamedDecl *Member) {
>   if (FieldDecl *FD = dyn_cast<FieldDecl>(Member))
>     if (CXXRecordDecl *RD =
> @@ -1033,11 +1033,23 @@
>         Context.getCanonicalType(Context.getTypeDeclType(RD));
>       if (!DestType->isDependentType() &&
>           !From->getType()->isDependentType()) {

Sorry I didn't think of this before, but an early exit to reduce  
nesting would make this clearer:

   if (DestType->isDependentType() || From->isTypeDependent())
     return false;

> -        if (From->getType()->getAsPointerType())
> +        QualType FromRecordType = From->getType();
> +        QualType DestRecordType = DestType;
> +        if (FromRecordType->getAsPointerType()) {
>           DestType = Context.getPointerType(DestType);
> +          FromRecordType = FromRecordType->getPointeeType();
> +        }
> +        if (IsDerivedFrom(FromRecordType, DestRecordType) &&
> +            CheckDerivedToBaseConversion(FromRecordType,
> +                                     DestRecordType,
> +                                     From->getSourceRange().getBegin 
> (),
> +                                     From->getSourceRange()))
> +          return true;
> +

The IsDerivedFrom check can be expensive, so we'd like to avoid doing  
it twice (once in IsDerivedFrom, once in  
CheckDerivedToBaseConversion). Since the object conversion will alway  
have either (canonical) FromRecordType == (canonical) DestRecordType  
or FromRecordType derived from DestRecordType, why not check for the  
equality condition and, if the types aren't equal, just call  
CheckDerivedtoBaseConversion directly? It will assert that there  
actually was a derivation relationship.

>         ImpCastExprToType(From, DestType, /*isLvalue=*/true);


Also: could you add a test case that illustrates the need for  
CheckDerivedToBaseConversion?

	- Doug