[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:

> Hello Nicola,
> 
>    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 :)
> 

I agree. Actually, those arguments are explicitly
specified nearly everywhere in the code, so I can remove the default arguments as part of my
patch if you want.

> @@ -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 *')))))
> 


Yeah, my patch should've fixed also this example. It doesn't because I've not
changed ImpCastExprToType to CastExprToType in a couple of case labels in PerformConversion.
This example is fixed by changing it in the CCK_QualificationConversion case.
There are a couple of cases in complex conversions cases that I think have to be changed too,
but I'm not sure about the ArrayToPointer and FunctionToPointer cases, those in the
first pass of the conversion. I think they should remain implicit casts in any case.

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

Yes it is. CastExprToType creates explicit cast nodes without source locations and sourcetypeinfos, so if that cast node has a null
sourcetypeinfo, it's the one created by PerformConversion.

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

What kind of direct way do you suggest? I don't think a flag in CastOperation is a good idea because PerformConversion is not
bound to CastOperation. 

Also, I'm not sure about the assertion on Op.Kind vs Cast->getCastKind(). 
In common cases, the TryXXXCast function set Op.Kind, and then BuildXXXCastExpr
creates the node with that kind. However, when TryStaticImplicitCast is called,
it doesn't set Op.Kind, and the node is not created by BuildXXXCastExpr.
Actually, it always set the kind to CK_NoOp for things other than ConstructorConversions.
I could add a CK_AlreadySet enum with the only purpose of signaling that the node
has already been created?

> Oh, and '{'s up on the line above, rather than on their own line, please.

Ok, sure.

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

I've attached a single patch for those little changes.

> 	- Doug
> 
Thanks,
Nicola

-------------- next part --------------
A non-text attachment was scrubbed...
Name: InitializationKind.patch
Type: application/octet-stream
Size: 1882 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111025/c2114513/attachment.obj>