3 files changed, 52 insertions, 32 deletions

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index c54fc40ecb2..e9903df6128 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -870,10 +870,26 @@ bool Expr::isIntegerConstantExpr(llvm::APSInt &Result, ASTContext &Ctx,
     const BinaryOperator *Exp = cast<BinaryOperator>(this);
     llvm::APSInt LHS, RHS;
 
+    // Comma operator requires special handling.
+    if (Exp->getOpcode() == BinaryOperator::Comma) {
+      // C99 6.6p3: "shall not contain assignment, ..., or comma operators,
+      // *except* when they are contained within a subexpression that is not
+      // evaluated".  Note that Assignment can never happen due to constraints
+      // on the LHS subexpr, so we don't need to check it here.
+      if (isEvaluated) {
+        if (Loc) *Loc = getLocStart();
+        return false;
+      }
+
+      // The result of the constant expr is the RHS.
+      return Exp->getRHS()->isIntegerConstantExpr(Result, Ctx, Loc, 
+                                                  isEvaluated);
+    }
+    
     // Initialize result to have correct signedness and width.
     Result = llvm::APSInt(static_cast<uint32_t>(Ctx.getTypeSize(getType())),
-                          !getType()->isSignedIntegerType());                          
-    
+                          !getType()->isSignedIntegerType());
+
     // The LHS of a constant expr is always evaluated and needed.
     if (!Exp->getLHS()->isIntegerConstantExpr(LHS, Ctx, Loc, isEvaluated))
       return false;
@@ -945,20 +961,6 @@ bool Expr::isIntegerConstantExpr(llvm::APSInt &Result, ASTContext &Ctx,
     case BinaryOperator::LOr:
       Result = LHS != 0 || RHS != 0;
       break;
-      
-    case BinaryOperator::Comma:
-      // C99 6.6p3: "shall not contain assignment, ..., or comma operators,
-      // *except* when they are contained within a subexpression that is not
-      // evaluated".  Note that Assignment can never happen due to constraints
-      // on the LHS subexpr, so we don't need to check it here.
-      if (isEvaluated) {
-        if (Loc) *Loc = getLocStart();
-        return false;
-      }
-      
-      // The result of the constant expr is the RHS.
-      Result = RHS;
-      return true;
     }
     
     assert(!Exp->isAssignmentOp() && "LHS can't be a constant expr!");
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index d6a4094520d..f5477f30b30 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -376,6 +376,7 @@ public:
   }
   bool VisitDeclRefExpr(const DeclRefExpr *E);
   bool VisitCallExpr(const CallExpr *E);
+  bool VisitBinOpComma(const BinaryOperator *E);
   bool VisitBinaryOperator(const BinaryOperator *E);
   bool VisitUnaryOperator(const UnaryOperator *E);
 
@@ -480,7 +481,37 @@ bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
   }
 }
 
+bool IntExprEvaluator::VisitBinOpComma(const BinaryOperator *E) {
+  llvm::APSInt RHS(32);
+
+  // Require that we be able to evaluate the LHS.
+  if (!E->getLHS()->isEvaluatable(Info.Ctx))
+    return false;
+
+  bool OldEval = Info.isEvaluated;
+  if (!EvaluateInteger(E->getRHS(), RHS, Info))
+    return false;
+  Info.isEvaluated = OldEval;
+
+  // Result of the comma is just the result of the RHS.
+  Result = RHS;
+  
+  // C99 6.6p3: "shall not contain assignment, ..., or comma operators,
+  // *except* when they are contained within a subexpression that is not
+  // evaluated".  Note that Assignment can never happen due to constraints
+  // on the LHS subexpr, so we don't need to check it here.
+  if (!Info.isEvaluated)
+    return true;
+
+  // If the value is evaluated, we can accept it as an extension.
+  return Extension(E->getOperatorLoc(), diag::ext_comma_in_constant_expr);
+}
+
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
+  // Comma operator requires special handling.
+  if (E->getOpcode() == BinaryOperator::Comma)
+    return VisitBinOpComma(E);
+  
   // The LHS of a constant expr is always evaluated and needed.
   llvm::APSInt RHS(32);
   if (!Visit(E->getLHS())) {
@@ -585,22 +616,7 @@ bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
     Result = Result != 0 || RHS != 0;
     Result.zextOrTrunc(getIntTypeSizeInBits(E->getType()));
     break;
-      
-    
-  case BinaryOperator::Comma:
-    // Result of the comma is just the result of the RHS.
-    Result = RHS;
-
-    // C99 6.6p3: "shall not contain assignment, ..., or comma operators,
-    // *except* when they are contained within a subexpression that is not
-    // evaluated".  Note that Assignment can never happen due to constraints
-    // on the LHS subexpr, so we don't need to check it here.
-    if (!Info.isEvaluated)
-      return true;
-      
-    // If the value is evaluated, we can accept it as an extension.
-    return Extension(E->getOperatorLoc(), diag::ext_comma_in_constant_expr);
-  }
+}
 
   Result.setIsUnsigned(E->getType()->isUnsignedIntegerType());
   return true;
diff --git a/clang/test/Sema/const-eval.c b/clang/test/Sema/const-eval.c
index f42ed2504b1..c808b4818f7 100644
--- a/clang/test/Sema/const-eval.c
+++ b/clang/test/Sema/const-eval.c
@@ -11,3 +11,5 @@ struct y {int x,y;};
 EVAL_EXPR(6, (int)(1+(struct y*)0))
 EVAL_EXPR(7, (int)&((struct y*)0)->y)
 EVAL_EXPR(8, (_Bool)"asdf")
+void g0(void);
+EVAL_EXPR(9, (g0(), 12)) // expected-error {{fields must have a constant size}}