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

[Nicola Gigante]

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.

>> 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:442: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?

> - Richard
> 

Thanks,
Nicola

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ImplicitCastsRemoval-r147266.patch
Type: application/octet-stream
Size: 51800 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111226/e43d6ec7/attachment.obj>
-------------- next part --------------