Convert CheckICE into a statment visitor

[void]

Summary

This cleans up the code somewhat and allows us conditionally to act on
different types of nodes depending on their context. E.g., if we're
checking for an ICE in a constant context.

[void]

Ping?

[rsmith]

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

[void]

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

[rsmith]

In D54356#1297470, @rsmith wrote:

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

Oh, I see, you want to temporarily set "IsConstantContext" to true while evaluating the subexpression of a ConstantExpr. I think that's unnecessary, because anywhere we ask "is this syntactically an ICE?", we always want to evaluate as if in a constant context (unlike places where we run the evaluator on an expression, where that doesn't necessarily imply anything in particular about the nature of the expression.)

[void]

Okay. I'll revert this then.

[rsmith]

In D54356#1297506, @void wrote:

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

Thinking about this some more: in the case where adding a new Stmt node without considering this code is likely to result in a silent and initially-unnoticed bug, I think it's useful to use one of our covered-switch-like patterns. But I don't think this actually is such a case; the C ICE rules are pretty conservative in what they allow, and new Stmt nodes should nearly always be treated as non-ICE. Thank you!

[rsmith]

In D54356#1297522, @void wrote:

Okay. I'll revert this then.

I don't think we necessarily need the change in the other patch that's based on this one, but I still think this refactoring is an improvement :)

[void]

This isn't necessary. We can assume it's in a constant context because it's checking for an ICE.

[void]

In D54356#1297543, @rsmith wrote:

In D54356#1297522, @void wrote:

Okay. I'll revert this then.

I don't think we necessarily need the change in the other patch that's based on this one, but I still think this refactoring is an improvement :)

Thanks. I can resurrect this after these changes go in. This will keep the resulting changes small. :-)

lib/AST/ExprConstant.cpp

 };
 
 }
 
 static ICEDiag NoDiag() { return ICEDiag(IK_ICE, SourceLocation()); }
 
 static ICEDiag Worst(ICEDiag A, ICEDiag B) { return A.Kind >= B.Kind ? A : B; }
 
-static ICEDiag CheckEvalInICE(const Expr* E, const ASTContext &Ctx) {
+static ICEDiag CheckEvalInICE(const Expr *E, const ASTContext &Ctx,
+                              EvalInfo &Info) {
   Expr::EvalResult EVResult;
-  if (!E->EvaluateAsRValue(EVResult, Ctx) || EVResult.HasSideEffects ||
+  if (!::EvaluateAsRValue(Info, E, EVResult.Val) || EVResult.HasSideEffects ||
       !EVResult.Val.isInt())
     return ICEDiag(IK_NotICE, E->getBeginLoc());
 
   return NoDiag();
 }
 
-static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
-  assert(!E->isValueDependent() && "Should not see value dependent exprs!");
-  if (!E->getType()->isIntegralOrEnumerationType())
-    return ICEDiag(IK_NotICE, E->getBeginLoc());
+class ICEChecker
+  : public ConstStmtVisitor<ICEChecker, ICEDiag> {
+  const ASTContext &Ctx;
+  EvalInfo &Info;
 
+  typedef ConstStmtVisitor<ICEChecker, ICEDiag> StmtVisitorTy;
+
+public:
+  ICEChecker(const ASTContext &Ctx, EvalInfo &Info)
+      : Ctx(Ctx), Info(Info) {}
+
+  ICEDiag VisitStmt(const Stmt *S) {
+    return ICEDiag(IK_NotICE, S->getBeginLoc());
+  }
+
+  ICEDiag VisitExpr(const Expr *E);
+  ICEDiag VisitInitListExpr(const InitListExpr *E);
+  ICEDiag VisitSubstNonTypeTemplateParmExpr(
+      const SubstNonTypeTemplateParmExpr *E);
+  ICEDiag VisitParenExpr(const ParenExpr *E);
+  ICEDiag VisitGenericSelectionExpr(const GenericSelectionExpr *E);
+  ICEDiag VisitCallExpr(const CallExpr *E);
+  ICEDiag VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *E);
+  ICEDiag VisitDeclRefExpr(const DeclRefExpr *E);
+  ICEDiag VisitBinaryOperator(const BinaryOperator *E);
+  ICEDiag VisitUnaryOperator(const UnaryOperator *E);
+  ICEDiag VisitOffsetOfExpr(const OffsetOfExpr *E);
+  ICEDiag VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
+  ICEDiag VisitCastExpr(const CastExpr *E);
+  ICEDiag VisitBinaryConditionalOperator(const BinaryConditionalOperator *E);
+  ICEDiag VisitConditionalOperator(const ConditionalOperator *E);
+  ICEDiag VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E);
+  ICEDiag VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E);
+  ICEDiag VisitChooseExpr(const ChooseExpr *E);
+};
+
+ICEDiag ICEChecker::VisitExpr(const Expr *E) {
   switch (E->getStmtClass()) {
-#define ABSTRACT_STMT(Node)
-#define STMT(Node, Base) case Expr::Node##Class:
-#define EXPR(Node, Base)
-#include "clang/AST/StmtNodes.inc"
-  case Expr::PredefinedExprClass:
-  case Expr::FloatingLiteralClass:
-  case Expr::ImaginaryLiteralClass:
-  case Expr::StringLiteralClass:
-  case Expr::ArraySubscriptExprClass:
-  case Expr::OMPArraySectionExprClass:
-  case Expr::MemberExprClass:
-  case Expr::CompoundAssignOperatorClass:
-  case Expr::CompoundLiteralExprClass:
-  case Expr::ExtVectorElementExprClass:
-  case Expr::DesignatedInitExprClass:
-  case Expr::ArrayInitLoopExprClass:
-  case Expr::ArrayInitIndexExprClass:
-  case Expr::NoInitExprClass:
-  case Expr::DesignatedInitUpdateExprClass:
-  case Expr::ImplicitValueInitExprClass:
-  case Expr::ParenListExprClass:
-  case Expr::VAArgExprClass:
-  case Expr::AddrLabelExprClass:
-  case Expr::StmtExprClass:
-  case Expr::CXXMemberCallExprClass:
-  case Expr::CUDAKernelCallExprClass:
-  case Expr::CXXDynamicCastExprClass:
-  case Expr::CXXTypeidExprClass:
-  case Expr::CXXUuidofExprClass:
-  case Expr::MSPropertyRefExprClass:
-  case Expr::MSPropertySubscriptExprClass:
-  case Expr::CXXNullPtrLiteralExprClass:
-  case Expr::UserDefinedLiteralClass:
-  case Expr::CXXThisExprClass:
-  case Expr::CXXThrowExprClass:
-  case Expr::CXXNewExprClass:
-  case Expr::CXXDeleteExprClass:
-  case Expr::CXXPseudoDestructorExprClass:
-  case Expr::UnresolvedLookupExprClass:
-  case Expr::TypoExprClass:
-  case Expr::DependentScopeDeclRefExprClass:
-  case Expr::CXXConstructExprClass:
-  case Expr::CXXInheritedCtorInitExprClass:
-  case Expr::CXXStdInitializerListExprClass:
-  case Expr::CXXBindTemporaryExprClass:
-  case Expr::ExprWithCleanupsClass:
-  case Expr::CXXTemporaryObjectExprClass:
-  case Expr::CXXUnresolvedConstructExprClass:
-  case Expr::CXXDependentScopeMemberExprClass:
-  case Expr::UnresolvedMemberExprClass:
-  case Expr::ObjCStringLiteralClass:
-  case Expr::ObjCBoxedExprClass:
-  case Expr::ObjCArrayLiteralClass:
-  case Expr::ObjCDictionaryLiteralClass:
-  case Expr::ObjCEncodeExprClass:
-  case Expr::ObjCMessageExprClass:
-  case Expr::ObjCSelectorExprClass:
-  case Expr::ObjCProtocolExprClass:
-  case Expr::ObjCIvarRefExprClass:
-  case Expr::ObjCPropertyRefExprClass:
-  case Expr::ObjCSubscriptRefExprClass:
-  case Expr::ObjCIsaExprClass:
-  case Expr::ObjCAvailabilityCheckExprClass:
-  case Expr::ShuffleVectorExprClass:
-  case Expr::ConvertVectorExprClass:
-  case Expr::BlockExprClass:
-  case Expr::NoStmtClass:
-  case Expr::OpaqueValueExprClass:
-  case Expr::PackExpansionExprClass:
-  case Expr::SubstNonTypeTemplateParmPackExprClass:
-  case Expr::FunctionParmPackExprClass:
-  case Expr::AsTypeExprClass:
-  case Expr::ObjCIndirectCopyRestoreExprClass:
-  case Expr::MaterializeTemporaryExprClass:
-  case Expr::PseudoObjectExprClass:
-  case Expr::AtomicExprClass:
-  case Expr::LambdaExprClass:
-  case Expr::CXXFoldExprClass:
-  case Expr::CoawaitExprClass:
-  case Expr::DependentCoawaitExprClass:
-  case Expr::CoyieldExprClass:
+  default:
     return ICEDiag(IK_NotICE, E->getBeginLoc());
+  case Expr::ArrayTypeTraitExprClass:
+  case Expr::CharacterLiteralClass:
+  case Expr::CXXBoolLiteralExprClass:
+  case Expr::CXXNoexceptExprClass:
+  case Expr::CXXScalarValueInitExprClass:
+  case Expr::ExpressionTraitExprClass:
+  case Expr::FixedPointLiteralClass:
+  case Expr::GNUNullExprClass:
+    // GCC considers the GNU __null value to be an integral constant expression.
+  case Expr::IntegerLiteralClass:
+  case Expr::ObjCBoolLiteralExprClass:
+  case Expr::SizeOfPackExprClass:
+  case Expr::TypeTraitExprClass:
+    return NoDiag();
+  }
+}
 
-  case Expr::InitListExprClass: {
-    // C++03 [dcl.init]p13: If T is a scalar type, then a declaration of the
-    // form "T x = { a };" is equivalent to "T x = a;".
-    // Unless we're initializing a reference, T is a scalar as it is known to be
-    // of integral or enumeration type.
-    if (E->isRValue())
-      if (cast<InitListExpr>(E)->getNumInits() == 1)
-        return CheckICE(cast<InitListExpr>(E)->getInit(0), Ctx);
-    return ICEDiag(IK_NotICE, E->getBeginLoc());
-  }
+ICEDiag ICEChecker::VisitInitListExpr(const InitListExpr *E) {
+  // C++03 [dcl.init]p13: If T is a scalar type, then a declaration of the form
+  // "T x = { a };" is equivalent to "T x = a;".
+    if (E->getNumInits() == 1)
+      return StmtVisitorTy::Visit(E->getInit(0));
 
-  case Expr::SizeOfPackExprClass:
-  case Expr::GNUNullExprClass:
-    // GCC considers the GNU __null value to be an integral constant expression.
-    return NoDiag();
+ICEDiag ICEChecker::VisitSubstNonTypeTemplateParmExpr(
+    const SubstNonTypeTemplateParmExpr *E) {
+  return StmtVisitorTy::Visit(E->getReplacement());
+}
 
-  case Expr::SubstNonTypeTemplateParmExprClass:
-    return
-      CheckICE(cast<SubstNonTypeTemplateParmExpr>(E)->getReplacement(), Ctx);
-
-  case Expr::ConstantExprClass:
-    return CheckICE(cast<ConstantExpr>(E)->getSubExpr(), Ctx);
-
-  case Expr::ParenExprClass:
-    return CheckICE(cast<ParenExpr>(E)->getSubExpr(), Ctx);
-  case Expr::GenericSelectionExprClass:
-    return CheckICE(cast<GenericSelectionExpr>(E)->getResultExpr(), Ctx);
-  case Expr::IntegerLiteralClass:
-  case Expr::FixedPointLiteralClass:
-  case Expr::CharacterLiteralClass:
-  case Expr::ObjCBoolLiteralExprClass:
-  case Expr::CXXBoolLiteralExprClass:
-  case Expr::CXXScalarValueInitExprClass:
-  case Expr::TypeTraitExprClass:
-  case Expr::ArrayTypeTraitExprClass:
-  case Expr::ExpressionTraitExprClass:
-  case Expr::CXXNoexceptExprClass:
-    return NoDiag();
-  case Expr::CallExprClass:
-  case Expr::CXXOperatorCallExprClass: {
-    // C99 6.6/3 allows function calls within unevaluated subexpressions of
-    // constant expressions, but they can never be ICEs because an ICE cannot
-    // contain an operand of (pointer to) function type.
-    const CallExpr *CE = cast<CallExpr>(E);
-    if (CE->getBuiltinCallee())
-      return CheckEvalInICE(E, Ctx);
-    return ICEDiag(IK_NotICE, E->getBeginLoc());
-  }
-  case Expr::DeclRefExprClass: {
-    if (isa<EnumConstantDecl>(cast<DeclRefExpr>(E)->getDecl()))
-      return NoDiag();
-    const ValueDecl *D = cast<DeclRefExpr>(E)->getDecl();
-    if (Ctx.getLangOpts().CPlusPlus &&
-        D && IsConstNonVolatile(D->getType())) {
-      // Parameter variables are never constants.  Without this check,
-      // getAnyInitializer() can find a default argument, which leads
-      // to chaos.
-      if (isa<ParmVarDecl>(D))
-        return ICEDiag(IK_NotICE, cast<DeclRefExpr>(E)->getLocation());
+ICEDiag ICEChecker::VisitParenExpr(const ParenExpr *E) {
+  return StmtVisitorTy::Visit(E->getSubExpr());
+}
+
+ICEDiag ICEChecker::VisitGenericSelectionExpr(const GenericSelectionExpr *E) {
+  return StmtVisitorTy::Visit(E->getResultExpr());
+}
+
+ICEDiag ICEChecker::VisitCallExpr(const CallExpr *E) {
+  if (E->getBuiltinCallee())
+    return CheckEvalInICE(E, Ctx, Info);
+
+ICEDiag ICEChecker::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *E) {
+  return VisitCallExpr(E);
+}
+
+ICEDiag ICEChecker::VisitDeclRefExpr(const DeclRefExpr *E) {
+  if (isa<EnumConstantDecl>(E->getDecl()))
+
+  const ValueDecl *D = E->getDecl();
+      return ICEDiag(IK_NotICE, E->getLocation());
 
-      // C++ 7.1.5.1p2
-      //   A variable of non-volatile const-qualified integral or enumeration
-      //   type initialized by an ICE can be used in ICEs.
-      if (const VarDecl *Dcl = dyn_cast<VarDecl>(D)) {
-        if (!Dcl->getType()->isIntegralOrEnumerationType())
-          return ICEDiag(IK_NotICE, cast<DeclRefExpr>(E)->getLocation());
+        return ICEDiag(IK_NotICE, E->getLocation());
 
-        const VarDecl *VD;
-        // Look for a declaration of this variable that has an initializer, and
-        // check whether it is an ICE.
-        if (Dcl->getAnyInitializer(VD) && VD->checkInitIsICE())
-          return NoDiag();
-        else
-          return ICEDiag(IK_NotICE, cast<DeclRefExpr>(E)->getLocation());
-      }
-    }
-    return ICEDiag(IK_NotICE, E->getBeginLoc());
-  }
-  case Expr::UnaryOperatorClass: {
-    const UnaryOperator *Exp = cast<UnaryOperator>(E);
-    switch (Exp->getOpcode()) {
-    case UO_PostInc:
-    case UO_PostDec:
-    case UO_PreInc:
-    case UO_PreDec:
-    case UO_AddrOf:
-    case UO_Deref:
-    case UO_Coawait:
-      // C99 6.6/3 allows increment and decrement within unevaluated
-      // subexpressions of constant expressions, but they can never be ICEs
-      // because an ICE cannot contain an lvalue operand.
-      return ICEDiag(IK_NotICE, E->getBeginLoc());
-    case UO_Extension:
-    case UO_LNot:
-    case UO_Plus:
-    case UO_Minus:
-    case UO_Not:
-    case UO_Real:
-    case UO_Imag:
-      return CheckICE(Exp->getSubExpr(), Ctx);
-    }
-    llvm_unreachable("invalid unary operator class");
-  }
-  case Expr::OffsetOfExprClass: {
-    // Note that per C99, offsetof must be an ICE. And AFAIK, using
-    // EvaluateAsRValue matches the proposed gcc behavior for cases like
-    // "offsetof(struct s{int x[4];}, x[1.0])".  This doesn't affect
-    // compliance: we should warn earlier for offsetof expressions with
-    // array subscripts that aren't ICEs, and if the array subscripts
-    // are ICEs, the value of the offsetof must be an integer constant.
-    return CheckEvalInICE(E, Ctx);
-  }
-  case Expr::UnaryExprOrTypeTraitExprClass: {
-    const UnaryExprOrTypeTraitExpr *Exp = cast<UnaryExprOrTypeTraitExpr>(E);
-    if ((Exp->getKind() ==  UETT_SizeOf) &&
-        Exp->getTypeOfArgument()->isVariableArrayType())
-      return ICEDiag(IK_NotICE, E->getBeginLoc());
-    return NoDiag();
-  }
-  case Expr::BinaryOperatorClass: {
-    const BinaryOperator *Exp = cast<BinaryOperator>(E);
-    switch (Exp->getOpcode()) {
-    case BO_PtrMemD:
-    case BO_PtrMemI:
-    case BO_Assign:
-    case BO_MulAssign:
-    case BO_DivAssign:
-    case BO_RemAssign:
-    case BO_AddAssign:
-    case BO_SubAssign:
-    case BO_ShlAssign:
-    case BO_ShrAssign:
-    case BO_AndAssign:
-    case BO_XorAssign:
-    case BO_OrAssign:
-      // C99 6.6/3 allows assignments within unevaluated subexpressions of
-      // constant expressions, but they can never be ICEs because an ICE cannot
-      // contain an lvalue operand.
-      return ICEDiag(IK_NotICE, E->getBeginLoc());
+        return ICEDiag(IK_NotICE, E->getLocation());
+
+ICEDiag ICEChecker::VisitUnaryOperator(const UnaryOperator *E) {
+  switch (E->getOpcode()) {
+
+    return StmtVisitorTy::Visit(E->getSubExpr());
+
+ICEDiag ICEChecker::VisitBinaryOperator(const BinaryOperator *E) {
+  switch (E->getOpcode()) {
 
-    case BO_Mul:
-    case BO_Div:
-    case BO_Rem:
-    case BO_Add:
-    case BO_Sub:
-    case BO_Shl:
-    case BO_Shr:
-    case BO_LT:
-    case BO_GT:
-    case BO_LE:
-    case BO_GE:
-    case BO_EQ:
-    case BO_NE:
-    case BO_And:
-    case BO_Xor:
-    case BO_Or:
-    case BO_Comma:
-    case BO_Cmp: {
-      ICEDiag LHSResult = CheckICE(Exp->getLHS(), Ctx);
-      ICEDiag RHSResult = CheckICE(Exp->getRHS(), Ctx);
-      if (Exp->getOpcode() == BO_Div ||
-          Exp->getOpcode() == BO_Rem) {
-        // EvaluateAsRValue gives an error for undefined Div/Rem, so make sure
-        // we don't evaluate one.
-        if (LHSResult.Kind == IK_ICE && RHSResult.Kind == IK_ICE) {
-          llvm::APSInt REval = Exp->getRHS()->EvaluateKnownConstInt(Ctx);
-          if (REval == 0)
-            return ICEDiag(IK_ICEIfUnevaluated, E->getBeginLoc());
-          if (REval.isSigned() && REval.isAllOnesValue()) {
-            llvm::APSInt LEval = Exp->getLHS()->EvaluateKnownConstInt(Ctx);
-            if (LEval.isMinSignedValue())
-              return ICEDiag(IK_ICEIfUnevaluated, E->getBeginLoc());
-          }
-        }
-      }
-      if (Exp->getOpcode() == BO_Comma) {
-        if (Ctx.getLangOpts().C99) {
-          // C99 6.6p3 introduces a strange edge case: comma can be in an ICE
-          // if it isn't evaluated.
-          if (LHSResult.Kind == IK_ICE && RHSResult.Kind == IK_ICE)
-            return ICEDiag(IK_ICEIfUnevaluated, E->getBeginLoc());
-        } else {
-          // In both C89 and C++, commas in ICEs are illegal.
-          return ICEDiag(IK_NotICE, E->getBeginLoc());
-        }
-      }
-      return Worst(LHSResult, RHSResult);
-    }
-    case BO_LAnd:
-    case BO_LOr: {
-      ICEDiag LHSResult = CheckICE(Exp->getLHS(), Ctx);
-      ICEDiag RHSResult = CheckICE(Exp->getRHS(), Ctx);
-      if (LHSResult.Kind == IK_ICE && RHSResult.Kind == IK_ICEIfUnevaluated) {
-        // Rare case where the RHS has a comma "side-effect"; we need
-        // to actually check the condition to see whether the side
-        // with the comma is evaluated.
-        if ((Exp->getOpcode() == BO_LAnd) !=
-            (Exp->getLHS()->EvaluateKnownConstInt(Ctx) == 0))
-          return RHSResult;
-        return NoDiag();
-      }
+    ICEDiag LHSResult = StmtVisitorTy::Visit(E->getLHS());
+    ICEDiag RHSResult = StmtVisitorTy::Visit(E->getRHS());
+    if (E->getOpcode() == BO_Div ||
+        E->getOpcode() == BO_Rem) {
+        llvm::APSInt REval = E->getRHS()->EvaluateKnownConstInt(Ctx);
+          llvm::APSInt LEval = E->getLHS()->EvaluateKnownConstInt(Ctx);
+    if (E->getOpcode() == BO_Comma) {
+    ICEDiag LHSResult = StmtVisitorTy::Visit(E->getLHS());
+    ICEDiag RHSResult = StmtVisitorTy::Visit(E->getRHS());
+      if ((E->getOpcode() == BO_LAnd) !=
+          (E->getLHS()->EvaluateKnownConstInt(Ctx) == 0))
 
-      return Worst(LHSResult, RHSResult);
-    }
-    }
-    llvm_unreachable("invalid binary operator kind");
-  }
-  case Expr::ImplicitCastExprClass:
-  case Expr::CStyleCastExprClass:
-  case Expr::CXXFunctionalCastExprClass:
-  case Expr::CXXStaticCastExprClass:
-  case Expr::CXXReinterpretCastExprClass:
-  case Expr::CXXConstCastExprClass:
-  case Expr::ObjCBridgedCastExprClass: {
-    const Expr *SubExpr = cast<CastExpr>(E)->getSubExpr();
-    if (isa<ExplicitCastExpr>(E)) {
-      if (const FloatingLiteral *FL
-            = dyn_cast<FloatingLiteral>(SubExpr->IgnoreParenImpCasts())) {
-        unsigned DestWidth = Ctx.getIntWidth(E->getType());
-        bool DestSigned = E->getType()->isSignedIntegerOrEnumerationType();
-        APSInt IgnoredVal(DestWidth, !DestSigned);
-        bool Ignored;
-        // If the value does not fit in the destination type, the behavior is
-        // undefined, so we are not required to treat it as a constant
-        // expression.
-        if (FL->getValue().convertToInteger(IgnoredVal,
-                                            llvm::APFloat::rmTowardZero,
-                                            &Ignored) & APFloat::opInvalidOp)
-          return ICEDiag(IK_NotICE, E->getBeginLoc());
-        return NoDiag();
-      }
-    }
-    switch (cast<CastExpr>(E)->getCastKind()) {
-    case CK_LValueToRValue:
-    case CK_AtomicToNonAtomic:
-    case CK_NonAtomicToAtomic:
-    case CK_NoOp:
-    case CK_IntegralToBoolean:
-    case CK_IntegralCast:
-      return CheckICE(SubExpr, Ctx);
-    default:
-      return ICEDiag(IK_NotICE, E->getBeginLoc());
-    }
-  }
-  case Expr::BinaryConditionalOperatorClass: {
-    const BinaryConditionalOperator *Exp = cast<BinaryConditionalOperator>(E);
-    ICEDiag CommonResult = CheckICE(Exp->getCommon(), Ctx);
-    if (CommonResult.Kind == IK_NotICE) return CommonResult;
-    ICEDiag FalseResult = CheckICE(Exp->getFalseExpr(), Ctx);
-    if (FalseResult.Kind == IK_NotICE) return FalseResult;
-    if (CommonResult.Kind == IK_ICEIfUnevaluated) return CommonResult;
-    if (FalseResult.Kind == IK_ICEIfUnevaluated &&
-        Exp->getCommon()->EvaluateKnownConstInt(Ctx) != 0) return NoDiag();
-    return FalseResult;
-  }
-  case Expr::ConditionalOperatorClass: {
-    const ConditionalOperator *Exp = cast<ConditionalOperator>(E);
-    // If the condition (ignoring parens) is a __builtin_constant_p call,
-    // then only the true side is actually considered in an integer constant
-    // expression, and it is fully evaluated.  This is an important GNU
-    // extension.  See GCC PR38377 for discussion.
-    if (const CallExpr *CallCE
-        = dyn_cast<CallExpr>(Exp->getCond()->IgnoreParenCasts()))
-      if (CallCE->getBuiltinCallee() == Builtin::BI__builtin_constant_p)
-        return CheckEvalInICE(E, Ctx);
-    ICEDiag CondResult = CheckICE(Exp->getCond(), Ctx);
-    if (CondResult.Kind == IK_NotICE)
-      return CondResult;
+
+ICEDiag ICEChecker::VisitOffsetOfExpr(const OffsetOfExpr *E) {
+  // Note that per C99, offsetof must be an ICE. And AFAIK, using
+  // EvaluateAsRValue matches the proposed gcc behavior for cases like
+  // "offsetof(struct s{int x[4];}, x[1.0])".  This doesn't affect compliance:
+  // we should warn earlier for offsetof expressions with array subscripts that
+  // aren't ICEs, and if the array subscripts are ICEs, the value of the
+  // offsetof must be an integer constant.
+  return CheckEvalInICE(E, Ctx, Info);
+}
+
+ICEDiag ICEChecker::VisitUnaryExprOrTypeTraitExpr(
+    const UnaryExprOrTypeTraitExpr *E) {
+  if (E->getKind() ==  UETT_SizeOf &&
+      E->getTypeOfArgument()->isVariableArrayType())
+    return ICEDiag(IK_NotICE, E->getBeginLoc());
+  return NoDiag();
+}
+
+ICEDiag ICEChecker::VisitCastExpr(const CastExpr *E) {
+  const Expr *SubExpr = E->getSubExpr();
+  switch (E->getCastKind()) {
+  default:
+    return ICEDiag(IK_NotICE, E->getBeginLoc());
+    return StmtVisitorTy::Visit(SubExpr);
+
+ICEDiag ICEChecker::VisitBinaryConditionalOperator(
+    const BinaryConditionalOperator *E) {
+  ICEDiag CommonResult = StmtVisitorTy::Visit(E->getCommon());
+  ICEDiag FalseResult = StmtVisitorTy::Visit(E->getFalseExpr());
+      E->getCommon()->EvaluateKnownConstInt(Ctx) != 0) return NoDiag();
+
+ICEDiag ICEChecker::VisitConditionalOperator(const ConditionalOperator *E) {
+      = dyn_cast<CallExpr>(E->getCond()->IgnoreParenCasts()))
+      return CheckEvalInICE(E, Ctx, Info);
+  ICEDiag CondResult = StmtVisitorTy::Visit(E->getCond());
 
-    ICEDiag TrueResult = CheckICE(Exp->getTrueExpr(), Ctx);
-    ICEDiag FalseResult = CheckICE(Exp->getFalseExpr(), Ctx);
+  ICEDiag TrueResult = StmtVisitorTy::Visit(E->getTrueExpr());
+  ICEDiag FalseResult = StmtVisitorTy::Visit(E->getFalseExpr());
 
-    if (TrueResult.Kind == IK_NotICE)
-      return TrueResult;
-    if (FalseResult.Kind == IK_NotICE)
-      return FalseResult;
-    if (CondResult.Kind == IK_ICEIfUnevaluated)
-      return CondResult;
-    if (TrueResult.Kind == IK_ICE && FalseResult.Kind == IK_ICE)
-      return NoDiag();
-    // Rare case where the diagnostics depend on which side is evaluated
-    // Note that if we get here, CondResult is 0, and at least one of
-    // TrueResult and FalseResult is non-zero.
-    if (Exp->getCond()->EvaluateKnownConstInt(Ctx) == 0)
-      return FalseResult;
-    return TrueResult;
-  }
-  case Expr::CXXDefaultArgExprClass:
-    return CheckICE(cast<CXXDefaultArgExpr>(E)->getExpr(), Ctx);
-  case Expr::CXXDefaultInitExprClass:
-    return CheckICE(cast<CXXDefaultInitExpr>(E)->getExpr(), Ctx);
-  case Expr::ChooseExprClass: {
-    return CheckICE(cast<ChooseExpr>(E)->getChosenSubExpr(), Ctx);
-  }
-  }
+  if (E->getCond()->EvaluateKnownConstInt(Ctx) == 0)
+
+ICEDiag ICEChecker::VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E) {
+  return StmtVisitorTy::Visit(E->getExpr());
+}
+
+ICEDiag ICEChecker::VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E) {
+  return StmtVisitorTy::Visit(E->getExpr());
+
+ICEDiag ICEChecker::VisitChooseExpr(const ChooseExpr *E) {
+  return StmtVisitorTy::Visit(E->getChosenSubExpr());
 
-  llvm_unreachable("Invalid StmtClass!");
+static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
+  assert(!E->isValueDependent() && "Should not see value dependent exprs!");
+  if (!E->getType()->isIntegralOrEnumerationType())
+    return ICEDiag(IK_NotICE, E->getBeginLoc());
+
+  Expr::EvalStatus Status;
+  SmallVector<PartialDiagnosticAt, 8> Diags;
+  Status.Diag = &Diags;
+  EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpression);
+  return ICEChecker(Ctx, Info).Visit(E);
 }
 
 /// Evaluate an expression as a C++11 integral constant expression.
 static bool EvaluateCPlusPlus11IntegralConstantExpr(const ASTContext &Ctx,
                                                     const Expr *E,
                                                     llvm::APSInt *Value,
                                                     SourceLocation *Loc) {
   if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {