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

[Douglas Gregor]

On Oct 28, 2011, at 5:39 AM, Nicola Gigante wrote:

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

Okay.

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


Yes, this is looking good. The patch didn't quite apply cleanly; if you can bring it up to date with mainline I'll be happy to commit it, thanks!

Super-minor nit:

+    // CheckStaticCast _may_ have already created the cast node. Let's check
+    if(CastNodesCreated) {

Please put a space after the "if" and before the "(" (in a bunch of places).

	- Doug