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


Last month I've submitted a patch to address an issue
with redundant NoOp implicit casts in the AST.
You can find the previous thread here:

The patch I came up with were accepted and committed
but later reverted because of a bug on _Complex casts
found by Richard Smith, which is now fixed.
I've been waiting a little for Richard to tell me more
about another miscompilation that he found in Chrome
but I haven't received anything so far, so I'm asking
again to review my patch.

However, this is not the same patch as before.
In the previous patch, CastExprToType used to create
cast nodes with empty TypeSourceInfo's that had to
be filled later by callers. As Richard suggested,
this wasn't the most robust way to do it.
I've rewritten the patch to pass the relevant information
about TypeSourceInfo's directly down to CastExprToType,
and now the patch should be robust enough.
I also suspect that, since Richard's problems had to do with
null TypeSourceInfo's, this patch resolves his problems
as well.

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?

Thank you,

ImplicitCastsRemoval-r147266.patch (43.3 KB)

Forget what I said on the last mail. I was wrong.
Now I've fixed the patch. It passes all the tests an applies to
the latest revision (r150076).
If it's ok, I'd commit it :slight_smile:

Thank you,

ImplicitCastsRemoval-r150076.patch (53.8 KB)

Can I commit the patch?

I'm attaching a new one that applies to the latest revision (150387).


ImplicitCastsRemoval-r150387.patch (56.6 KB)

Is anybody reviewing my patch?

Someone is working on the same piece of code (especially SemaCast.cpp).
This means that every two revisions my patch doesn't apply anymore.

Can I commit the patch and go ahead?

Thank you,


I've attached an updated patch that applies to the current
revision, just in case.


ImplicitCastsRemoval-r151277.patch (56.8 KB)

For what it's worth, this looks like good stuff from an AST fidelity
perspective (I'm not sure I've given the actual implementation enough
review to have an opinion on whether it's the right approach, etc).
Here's one minor cleanup that we can do because of this change:

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index a89e813..fe3a8a6 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -4234,7 +4234,7 @@ void AnalyzeImplicitConversions(Sema &S, Expr
*OrigE, SourceLocation CC) {

   // Skip past explicit casts.
   if (isa<ExplicitCastExpr>(E)) {
- E = cast<ExplicitCastExpr>(E)->getSubExpr()->IgnoreParenImpCasts();
+ E = cast<ExplicitCastExpr>(E)->getSubExpr();
     return AnalyzeImplicitConversions(S, E, CC);

(this came to my attention since I've been dabbling in this code to
try to fix the warn_impcast_null_pointer_to_integer warning & thus
playing around in lots of this SemaChecking.cpp code)

No doubt there's other places that have to explicitly walk through the
implicit casts inside an explicit cast to perform similar work. Also,
I wonder, is it ever possible for an explicit cast to cause/have
implicit casts within it? Previously we wouldn't've been able to
understand the difference there (because all the implicit casts under
the noop explicit cast would be indistinguishable from one another).

- David