[cfe-commits] [Request for review] Removing redundant implicit casts in the AST

[Nicola Gigante]

Il giorno 24/ott/2011, alle ore 17:22, Douglas Gregor ha scritto:

>  ExprResult ImpCastExprToType(Expr *E, QualType Type, CastKind CK,
>                                ExprValueKind VK = VK_RValue,
> -                               const CXXCastPath *BasePath = 0,
> -                               CheckedConversionKind CCK 
> -                                  = CCK_ImplicitConversion);
> +                               const CXXCastPath *BasePath = 0);
> 
> I'm slightly nervous about the two defaulted arguments here, since '0' can be silently passed for either of them, but I guess it works since '0' is CCK_ImplicitConversion anyway :)
> 
> @@ -2193,13 +2192,13 @@
>   return Owned(From);
> }
> 
> -/// PerformImplicitConversion - Perform an implicit conversion of the
> +/// PerformConversion - Perform a conversion of the
> /// expression From to the type ToType by following the standard
> /// conversion sequence SCS. Returns the converted
> /// expression. Flavor is the context in which we're performing this
> /// conversion, for use in error messages.
> ExprResult
> -Sema::PerformImplicitConversion(Expr *From, QualType ToType,
> +Sema::PerformConversion(Expr *From, QualType ToType,
>                                 const StandardConversionSequence& SCS,
>                                 AssignmentAction Action, 
>                                 CheckedConversionKind CCK) {
> @@ -2277,20 +2276,20 @@
>   case ICK_Array_To_Pointer:
>     FromType = Context.getArrayDecayedType(FromType);
>     From = ImpCastExprToType(From, FromType, CK_ArrayToPointerDecay, 
> -                             VK_RValue, /*BasePath=*/0, CCK).take();
> +                             VK_RValue, /*BasePath=*/0).take();
>     break;
> 
>   case ICK_Function_To_Pointer:
>     FromType = Context.getPointerType(FromType);
>     From = ImpCastExprToType(From, FromType, CK_FunctionToPointerDecay, 
> -                             VK_RValue, /*BasePath=*/0, CCK).take();
> +                             VK_RValue, /*BasePath=*/0).take();
>     break;
> 
>   default:
>     llvm_unreachable("Improper first standard conversion");
>   }
> 
> I think you'll want to count the # of necessary conversions a bit more closely here, or maybe compute the number ahead of time. This still creates an extra conversion on code like this:
> 
> struct A { };
> 
> void f(A as[]) {
>  static_cast<const A*>(as);
> }
> 
> by producing
> 
>  (CXXStaticCastExpr 0x7f823203dde8 <line:4:3, col:27> 'const struct A *' static_cast<const struct A *> <NoOp>
>    (ImplicitCastExpr 0x7f823203ddd0 <col:25> 'const struct A *' <NoOp>
>      (ImplicitCastExpr 0x7f823203ddb8 <col:25> 'struct A *' <LValueToRValue>
>        (DeclRefExpr 0x7f823203dd50 <col:25> 'struct A *' lvalue ParmVar 0x7f823203dbf0 'as' 'struct A *')))))
> 
> Index: lib/Sema/SemaCast.cpp
> ===================================================================
> --- lib/Sema/SemaCast.cpp	(revision 142579)
> +++ lib/Sema/SemaCast.cpp	(working copy)
> @@ -289,6 +289,18 @@
>         return ExprError();
>     }
> 
> +    // CheckStaticCast _may_ have already created the cast node. Let's check
> +    if(CXXStaticCastExpr *Cast = dyn_cast<CXXStaticCastExpr>(Op.SrcExpr.get()))
> +    {
> +      if(!Cast->getTypeInfoAsWritten())
> +      {
> +        Cast->setTypeInfoAsWritten(DestTInfo);
> +        Cast->setOperatorLoc(OpLoc);
> +        Cast->setRParenLoc(Parens.getEnd());
> +        return Op.complete(Cast);
> +      }
> +    }
> +    
> 
> This feels a little shaky. I assume that the inner "if" is intended to make sure we don't cannibalize the inner "if" for something like
> 
> 	static_cast<int>(static_cast<float>(something))
> 
> However, I'd feel far better if we had a more direct way to signal to this code that the CXXStaticCastExpr has already been created. Also, it seems like some assertions comparing Op.Kind to Cast->getCastKind() and comparing the resulting types would be helpful to flush out any remaining issues.
> 
> Oh, and '{'s up on the line above, rather than on their own line, please.
> 
> The changes to Initialization.h (e.g., SIK_DirectCast -> SIK_DirectStaticCast and related) could go in at any time, if you'd like to tease these out from the main part of the patch.
> 

Hello.
I've attached a patch that corrects what you pointed out. The issue about FunctionToPointer and ArrayToPointer decays persists,
because they're handled in a very different way and they'll require a future new patch.
I've removed the "shaky if" by setting a boolean parameter in CheckStaticCast and CheckCXXCStyleCast, telling the caller
if the function have created and applied a new cast node.
I believe that this patch does what we need and is ready to be committed if you agree.
It applies to r143190 (today) and passes the tests.

> 	- Doug
> 

Thanks,
Nicola

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cast.patch
Type: application/octet-stream
Size: 31610 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111028/5a2939cb/attachment.obj>