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

[Richard Smith]

On Mon, December 26, 2011 18:03, Nicola Gigante wrote:
> Il giorno 25/dic/2011, alle ore 17:59, Richard Smith ha scritto:
>> Hi,
>>
>> The patch seems very good. There are a few superficial issues, but
>> functionally it looks great.
>
> Thank you.
> I've attached a version of the patch that address the issues you've pointed
> out, except one:
>
>> Index: lib/Sema/SemaCast.cpp
>> ===================================================================
>> --- lib/Sema/SemaCast.cpp        (revision 147266)
>> +++ lib/Sema/SemaCast.cpp        (working copy)
>> @@ -283,12 +285,22 @@
>> Parens.getEnd()));
>> }
>> case tok::kw_static_cast: { +    bool CastNodesCreated = false;
>> +    CheckedConversionInfo CCI(Sema::CCK_StaticCast,
>> +                              OpLoc, Parens.getEnd(), DestTInfo);
>> if (!TypeDependent) { -      Op.CheckStaticCast();
>> +      Op.CheckStaticCast(CastNodesCreated, CCI);
>> if (Op.SrcExpr.isInvalid()) return ExprError(); }
>>
>>
>> +    // CheckStaticCast _may_ have already created the cast node. Let's
>> check +    if (CastNodesCreated) {
>> +      if (CXXStaticCastExpr *Cast =
>> +          dyn_cast<CXXStaticCastExpr>(Op.SrcExpr.get())) {
>> +        return Op.complete(Cast);
>> +      }
>> +    }
>>
>>
>> Is the inner 'if' here necessary? If CastNodesCreated is true, it looks
>> like the node created should always be a CXXStaticCastExpr. If that's the
>> case, just use a cast<> rather than a dyn_cast<> and an 'if'. (And in the
>> other similar cases.)
>>
>
> Unfortunately, this is not the case. The CastNodesCreated flag is set in
> CheckStaticCast() and CheckCXXCStyleCast() methods if TryStaticCast
> modifies the expression:
>
> CastNodesCreated = (SrcExpr.get() != SrcExprOrig &&
> Kind != CK_ConstructorConversion);
>
>
> However, TryStaticCast in some cases rely on InitializationSequence::Perform
> to do its work, and it's not certain at all that the result is a CastNode. In
> this sense, the CastNodesCreated flag is just an index that something
> happened, but the dyn_cast is still needed. I tried to solve the issue,
> substituting the dyn_cast with an assert to see what could happen. This led me
> to the discovery of two cases where the patch failed to suppress the implicit
> casts I want to remove. I've now fixed these cases but I still get failures
> for that assertion. For example, I get an assert failure with this code:
>
>
> template<typename T> T f(T);
>
> void func() { (void)((int (*)(int))&f);
> }
>
>
> The important thing here is the template: the assertion doesn't fail if f()
> is a common non-template function.

Ah. What's happening here is that the unary & expression is being rewritten
because the cast resolves the overloaded function reference 'f' to a specific
function, which is tricking your check into thinking a cast has been created.

>>> It passes all the tests, except for two c-index tests that
>>> I don't know how to handle, because I don't
>>> know how to use c-index-test at all. I suspect they simply mean that the
>>> tests have to be modified for the new structure of the AST, but I don't
>>> know how. Can you point me in the right direction?
>>>
>>
>> Take a look at the c-index-test output on those testcases, make sure it
>> matches the AST you expected to build for them, then update the CHECK: lines
>>  in the testcases to match the output. If you get output from c-index-test
>> which you can't figure out, ask on cfe-dev or IRC and someone should be
>> able to help.
>>
>
> I've tried to understand what's going on here, but I'm not sure.
> One of the Index tests that fail is recursive-cxx-member-calls.cpp
> The output of FileCheck is:
>
>
> /Users/gigabytes/llvm/tools/clang/test/Index/recursive-cxx-member-calls.cpp:4
> 42:18: error: expected string not found in input
> // CHECK-tokens: Identifier: "size_t" [41:31 - 41:37] TypeRef=size_t:2:25
> ^
> <stdin>:256:1: note: scanning from here
> Identifier: "size_t" [41:31 - 41:37] UnaryOperator=
> ^
> <stdin>:278:1: note: possible intended match here
> Identifier: "size_t" [45:31 - 45:37] TypeRef=size_t:2:25
> ^
>
>
> The piece of code that fails is at line 41:
> static const size_t npos = ~size_t(0);
>
> and the output of c-index-test for that line with my patch is: Keyword:
> "static" [41:3 - 41:9] ClassDecl=StringRef:38:7 (Definition)
> Keyword: "const" [41:10 - 41:15] ClassDecl=StringRef:38:7 (Definition)
> Identifier: "size_t" [41:16 - 41:22] TypeRef=size_t:2:25
> Identifier: "npos" [41:23 - 41:27] VarDecl=npos:41:23
> Punctuation: "=" [41:28 - 41:29] VarDecl=npos:41:23
> Punctuation: "~" [41:30 - 41:31] UnaryOperator=
> Identifier: "size_t" [41:31 - 41:37] UnaryOperator=
> Punctuation: "(" [41:37 - 41:38] CXXFunctionalCastExpr=
> Literal: "0" [41:38 - 41:39] IntegerLiteral=
> Punctuation: ")" [41:39 - 41:40] CXXFunctionalCastExpr=
> Punctuation: ";" [41:40 - 41:41] ClassDecl=StringRef:38:7 (Definition)
>
>
> The "size_t" token was annotated as TypeRef and is now annotated as
> "UnaryOperator",
> which sounds wrong to me, but I can't understand why this happens.
>
> Any idea?

Take a look at the output of clang -Xclang -ast-dump-xml -S -o -. Testcase:

typedef unsigned size_t; size_t n = ~size_t(0);

Before:

  (CXXFunctionalCastExpr 0x5b08100 <col:38, col:46> 'size_t':'unsigned int'
functional cast to size_t <NoOp>
    (ImplicitCastExpr 0x5f7b0e8 <col:45> 'size_t':'unsigned int' <IntegralCast>

After:

  (CXXFunctionalCastExpr 0x58ae778 <col:44, col:46> 'size_t':'unsigned int'
functional cast to size_t <IntegralCast>

On the one hand, the cast expression is fixed. On the other, the source range
for this cast is now incorrect. It was:

typedef unsigned size_t; size_t n = ~size_t(0);
                                     ~~~~~~~~~

... and now is:

typedef unsigned size_t; size_t n = ~size_t(0);
                                           ~~~

- Richard