[cfe-commits] [cfe-dev] [review request] Removing redundant implicit casts in the AST, take 2

[David Blaikie]

Bumping this because I think it's a neat thing to tidy up.

I've attached a patch that's updated for r155507. The only really
weird thing was a chunk of code in RewriteModernObjC.cpp that seemed
like it was doing strictly the wrong thing for casts (& perhaps was
only working as a no-op since explicit casts never did anything) &
removing it fixed the break caused by this patch & didn't regress
anything else.

- David

On Sun, Mar 25, 2012 at 6:27 AM, Nicola Gigante
<nicola.gigante at gmail.com> wrote:
>
> Ping?
>
> Il giorno 24/feb/2012, alle ore 12:09, Nicola Gigante ha scritto:
>
>>
>> Il giorno 24/feb/2012, alle ore 03:28, Richard Smith ha scritto:
>>
>>> Hi,
>>>
>> Hello!
>>
>>> On to the patch. I think it's really very close now:
>>> [..]
>> I'm going to fix all those issues, off course.
>> Just a couple of things:
>>
>>> -void CastOperation::CheckStaticCast() {
>>> +void CastOperation::CheckStaticCast(bool &CastNodesCreated,
>>> +                                    Sema::CheckedConversionInfo CCI) {
>>>
>>> Rather than taking a bool argument by reference, it might be nicer to return a bool from here to indicate whether a CXXStaticCast node was created.
>>>
>> I've chosen to use a by-reference argument for clarity. A bool returned by a function named CheckStaticCast() can be very easily misinterpreted,
>> in my opinion, to mean "checking done" vs "checking failed", even adding a comment that says otherwise. Don't you think so?
>> I'll change it anyway, if you wish.
>>
>>
>>> +  CastNodesCreated = (SrcExpr.get() != SrcExprOrig &&
>>> +                      Kind != CK_ConstructorConversion);
>>>
>>> This approach makes me nervous: it seems too easy for us to accidentally change SrcExpr without building a cast node (checkNonOverloadPlaceholders could do this, for instance). Can we ensure that TryStaticCast returns TC_Success exactly when it's created a CXXStaticCast node, then use that to determine whether we need to build one?
>>>
>>
>> You're right. I think we can be sure that TryStaticImplicitCast() has created the node if it returns TC_Success and Kind != CK_ConstructorConversion.
>> But it depends on how InitializationSequence::Perform() behaves. Do you think this is the case?
>>
>>> In the ElType == ToType case, it looks like this could still produce a no-op static cast containing an implicit cast.
>>
>> You're right. Fixed it.
>>
>> I've also contextually fixed a warning that appears in addFixitForObjCARCConversion() that doesn't include
>> the recently added CCK_StaticCast enum value in a switch statement.
>>
>> Attached is the patch to the last revision with all the issues fixed excepting the two questions I asked here.
>>
>>>
>>> Thanks!
>>> Richard
>>
>> Thanks,
>> Nicola
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ImplicitCastsRemove-r155507.patch
Type: application/octet-stream
Size: 66024 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120424/7869a98d/attachment.obj>