[cfe-commits] [Request for review] Removing redundant implicit casts in the AST

[Douglas Gregor]

Hello Nicola,

On Oct 22, 2011, at 8:21 AM, Nicola Gigante wrote:

> Hello @clang
> 
> As mentioned in a previous thread early this month, I was working
> on fixing a glitch in the AST representation of some kind of casts.
> 
> To recall, consider this example:
> 
> int func() {
>  return int(0.5);
> }
> 
> Clang currently builds an AST like this:
> int func() (CompoundStmt 
> (ReturnStmt 
>   (CXXFunctionalCastExpr 'int' functional cast to int <NoOp>
>     (ImplicitCastExpr  'int' <FloatingToIntegral>
>       (FloatingLiteral  'double' 5.000000e-01)))))
> 
> As you can see, there's a spurious ImplicitCastExpr that imho shouldn't exist,
> while the explicit cast node is marked as NoOp.
> This behavior happens every time the user explicitly writes a cast
> that could have been done implicitly, not only with functional casts
> but also with CStyle and static casts.
> This bogus implicit cast node introduces various problems with clients that need an
> high degree of source-code fidelity in the AST.
> 
> Here is a patch that address the problem. The AST generated now for the previous
> example is:
> 
> int func() (CompoundStmt 
> (ReturnStmt 
>   (CXXFunctionalCastExpr  'int' functional cast to int <FloatingToIntegral>
>     (FloatingLiteral 'double' 5.000000e-01))))
> 
> This way, the AST is more compact and closer to the actual source code.
> To fix this, I've modified one of the overloads of
> the PerformImplicitConversion function.
> Previously, it had a CheckedConversionKind argument that was passed
> down to ImpCastExprToType(), which however always ignored it.
> Now, that overload of PerformImplicitConversion is called PerformConversion,
> and creates the right kinds of cast nodes following the CCK parameter.
> 
> The patch applies cleanly to a fairly recent clang revision (142493, a couple
> of days ago), and passes all the tests.
> 
> I'd like to receive your feedback about both the issue itself and the
> implementation of the fix.

   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.

	- Doug