[cfe-commits] r62971 - in /cfe/trunk: include/clang/Basic/DiagnosticKinds.def lib/Sema/Sema.h lib/Sema/SemaExprCXX.cpp lib/Sema/SemaOverload.cpp lib/Sema/SemaType.cpp test/SemaCXX/member-pointer.cpp test/SemaCXX/qualification-conversion.cpp

[Chris Lattner]

On Jan 25, 2009, at 11:43 AM, Sebastian Redl wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=62971&view=rev
> Log:
> Implement implicit conversions for pointers-to-member.

Hi Sebastian,

Time for some more random nit picking of code :)

> +/// CheckMemberPointerConversion - Check the member pointer  
> conversion from the
> +/// expression From to the type ToType. This routine checks for  
> ambiguous or
> +/// virtual (FIXME: or inaccessible) base-to-derived member pointer  
> conversions
> +/// for which IsMemberPointerConversion has already returned true.  
> It returns
> +/// true and produces a diagnostic if there was an error, or  
> returns false
> +/// otherwise.
> +bool Sema::CheckMemberPointerConversion(Expr *From, QualType  
> ToType) {
> +  QualType FromType = From->getType();
> +
> +  if (const MemberPointerType *FromPtrType =
> +        FromType->getAsMemberPointerType()) {
> +    if (const MemberPointerType *ToPtrType =
> +          ToType->getAsMemberPointerType()) {


How about:

   const MemberPointerType *FromPtrType = FromType- 
 >getAsMemberPointerType();
   if (FromPtrType == 0) return false;
   const MemberPointerType *ToPtrType = ToType->getAsMemberPointerType()
   if (ToPtrType == 0) return false;

?

Having large blocks of code indented like this makes me (as a reader)  
have to keep all the context in mind because I assume there is an  
"else".  By using early exit, I know exactly how these cases are  
handled without having to remember, or look ahead to the else.

> +      BasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/ 
> false,
> +                      /*DetectVirtual=*/true);
> +      bool DerivationOkay = IsDerivedFrom(ToClass, FromClass, Paths);
> +      assert(DerivationOkay &&
> +             "Should not have been called if derivation isn't OK.");
> +      if (!DerivationOkay)
> +        return true;

This 'if' (and others like it) are dead code, please remove it, the  
assertion should be enough.

Thanks,

-Chris