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

[Richard Smith]

Hi,

On Sun, December 25, 2011 15:05, Nicola Gigante wrote:
> 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:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20111017/048206.html
>
> 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.

The patch seems very good. There are a few superficial issues, but
functionally it looks great.

Index: include/clang/AST/ExprCXX.h
===================================================================
--- include/clang/AST/ExprCXX.h        (revision 147266)
+++ include/clang/AST/ExprCXX.h        (working copy)
@@ -178,9 +178,11 @@
   /// \brief Retrieve the location of the cast operator keyword, e.g.,
   /// "static_cast".
   SourceLocation getOperatorLoc() const { return Loc; }
+  void setOperatorLoc(SourceLocation const& OpLoc) { Loc = OpLoc; }

Please put the space to the left of the &. (There are a few other instances of
this).

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.)

@@ -302,7 +314,7 @@
 /// Try to diagnose a failed overloaded cast.  Returns true if
 /// diagnostics were emitted.
 static bool tryDiagnoseOverloadedCast(Sema &S, CastType CT,
-                                      SourceRange range, Expr *src,
+                                      Sema::CheckedConversionInfo CCI, Expr
*src,

This is just over 80 columns.

@@ -1853,8 +1874,14 @@
       (void)Fn;

     } else {
-      diagnoseBadCast(Self, msg, (FunctionalStyle ? CT_Functional : CT_CStyle),
-                      OpRange, SrcExpr.get(), DestType);
+      assert(CCI.getKind() == Sema::CCK_FunctionalCast ||
+             CCI.getKind() == Sema::CCK_CStyleCast);

Could this assert be moved up to the top of CheckCXXCStyleCast?

Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp        (revision 147266)
+++ lib/Sema/SemaExprCXX.cpp        (working copy)
@@ -2220,17 +2219,18 @@
   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,

Please line up the parameters to the open paren here.

Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp        (revision 147266)
+++ lib/Sema/SemaExpr.cpp        (working copy)
@@ -3980,9 +3980,10 @@

   InitializedEntity Entity
     = InitializedEntity::InitializeTemporary(literalType);
+  CheckedConversionInfo CCI(CCK_CStyleCast,LParenLoc,

Please put a space after this comma.

Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp        (revision 147266)
+++ lib/Sema/Sema.cpp        (working copy)
@@ -277,16 +280,45 @@
       MarkVTableUsed(E->getLocStart(),
                      cast<CXXRecordDecl>(RecordTy->getDecl()));
   }
-
-  if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
-    if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
-      ImpCast->setType(Ty);
-      ImpCast->setValueKind(VK);
-      return Owned(E);
-    }
+
+  switch(CCI.getKind()) {
+    default:
+      llvm_unreachable("Unexpected CheckedConversionKind");
+    case CCK_ImplicitConversion:
+      if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
+        if (ImpCast->getCastKind() == Kind && (!BasePath ||
BasePath->empty())) {
+          ImpCast->setType(Ty);
+          ImpCast->setValueKind(VK);
+          return Owned(E);
+        }
+      }
+      return Owned(ImplicitCastExpr::Create(Context, Ty, Kind, E, BasePath,
VK));

A couple of lines here are just over 80 columns.

> 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.

- Richard