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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
