Fix fallout from r219557

[asl]

Summary

Consider the following nifty 1 liner: (0 ? csqrtl(2.0f) : sqrtl(2.0f)). One can easily obtain such code from e.g. tgmath. Right now it produces an assertion because we fail to do the promotion real => _Complex real.

The case was properly handled previously (old handleOtherComplexFloatConversion routine), but was forgotten in the current version. This seems to be about fallout from r219557

[rsmith]

This change is incorrect. The usual arithmetic conversions do *not* take (_Complex double, double) to (_Complex double, _Complex double); note C11 6.13.1.8/1 ("... without change of type domain").

The bug lies in the conditional operator; per 6.5.15/5, "If both the second and third operands have arithmetic type, the result type that would be determined by the usual arithmetic conversions, were they applied to those two operands, is the type of the result." The problem is that CheckConditionalOperands discards the result of UsualArithmeticConversions (that is, the result type), and instead blindly uses the LHS type. The right place for this fix is there CheckConditionalOperands (and in CXXCheckConditionalOperands).

[asl]

The type is definitely ignored. However, for (a ? lhs : rhs) we should promote both lhs and rhs to their common type, right? And currently this is performed inside UsualArithmeticConversions (or, actually, inside handleComplexFloatConversion). Should we duplicate the behavior of handleComplexFloatConversion inside CheckConditionalOperands ?

[asl]

Just to make stuff clear.

Currently we're building the following AST:

`-FunctionDecl 0x7fc5b306d600 <line:4:1, line:7:1> line:4:5 main 'int (void)'
  `-CompoundStmt 0x7fc5b306d940 <col:16, line:7:1>
    |-ConditionalOperator 0x7fc5b306d8d0 <line:5:3, col:32> '_Complex long double'
    | |-IntegerLiteral 0x7fc5b306d6f8 <col:3> 'int' 0
    | |-CallExpr 0x7fc5b306d7a0 <col:7, col:17> 'long double'
    | | |-ImplicitCastExpr 0x7fc5b306d788 <col:7> 'long double (*)(long double)' <FunctionToPointerDecay>
    | | | `-DeclRefExpr 0x7fc5b306d718 <col:7> 'long double (long double)' Function 0x7fc5b3033e50 'sqrtl' 'long double (long double)'
    | | `-ImplicitCastExpr 0x7fc5b306d7d0 <col:13> 'long double' <FloatingCast>
    | |   `-FloatingLiteral 0x7fc5b306d740 <col:13> 'float' 2.000000e+00
    | `-CallExpr 0x7fc5b306d870 <col:21, col:32> '_Complex long double'
    |   |-ImplicitCastExpr 0x7fc5b306d858 <col:21> '_Complex long double (*)(_Complex long double)' <FunctionToPointerDecay>
    |   | `-DeclRefExpr 0x7fc5b306d7e8 <col:21> '_Complex long double (_Complex long double)' Function 0x7fc5b3033b30 'csqrtl' '_Complex long double (_Complex long double)'
    |   `-ImplicitCastExpr 0x7fc5b306d8b8 <col:28> '_Complex long double' <FloatingRealToComplex>
    |     `-ImplicitCastExpr 0x7fc5b306d8a0 <col:28> 'long double' <FloatingCast>
    |       `-FloatingLiteral 0x7fc5b306d810 <col:28> 'float' 2.000000e+00
    `-ReturnStmt 0x7fc5b306d920 <line:6:3, col:10>
      `-IntegerLiteral 0x7fc5b306d900 <col:10> 'int' 0

The type of operator itself is correct. However, the types of lhs and rhs do not match. Prior to r219557 there was additional ImplicitCastExpr to convert the result of sqrtl to _Complex long double.

[asl]

The AST before (e.g. generated by clang from XCode release) is:

`-FunctionDecl 0x102850b40 <line:4:1, line:7:1> main 'int (void)'
  `-CompoundStmt 0x102850e98 <line:4:16, line:7:1>
    |-ConditionalOperator 0x102850e28 <line:5:3, col:32> '_Complex long double'
    | |-IntegerLiteral 0x102850c38 <col:3> 'int' 0
    | |-ImplicitCastExpr 0x102850e10 <col:7, col:17> '_Complex long double' <FloatingRealToComplex>
    | | `-CallExpr 0x102850ce0 <col:7, col:17> 'long double'
    | |   |-ImplicitCastExpr 0x102850cc8 <col:7> 'long double (*)(long double)' <FunctionToPointerDecay>
    | |   | `-DeclRefExpr 0x102850c58 <col:7> 'long double (long double)' Function 0x102850970 'sqrtl' 'long double (long double)'
    | |   `-ImplicitCastExpr 0x102850d10 <col:13> 'long double' <FloatingCast>
    | |     `-FloatingLiteral 0x102850c80 <col:13> 'float' 2.000000e+00
    | `-CallExpr 0x102850db0 <col:21, col:32> '_Complex long double'
    |   |-ImplicitCastExpr 0x102850d98 <col:21> '_Complex long double (*)(_Complex long double)' <FunctionToPointerDecay>
    |   | `-DeclRefExpr 0x102850d28 <col:21> '_Complex long double (_Complex long double)' Function 0x10280c400 'csqrtl' '_Complex long double (_Complex long double)'
    |   `-ImplicitCastExpr 0x102850df8 <col:28> '_Complex long double' <FloatingRealToComplex>
    |     `-ImplicitCastExpr 0x102850de0 <col:28> 'long double' <FloatingCast>
    |       `-FloatingLiteral 0x102850d50 <col:28> 'float' 2.000000e+00
    `-ReturnStmt 0x102850e78 <line:6:3, col:10>
      `-IntegerLiteral 0x102850e58 <col:10> 'int' 0

Note the additional

-ImplicitCastExpr 0x102850e10 <col:7, col:17> '_Complex long double' <FloatingRealToComplex>

Prior to r219557 the conversion was done inside handleOtherComplexFloatConversion() which was a helper inside UsualArithmeticConversion(). Now the functionality was refactored into handleComplexFloatConversion(), but the it inserts conversions only for types of different orders (e.g. it would promote float to _Complex long double).

[asl]

Updated patch with explicit casts now posted. Ok?

[rsmith]

LGTM with accompanying testcases.

[asl]

Add testcase

[asl]

I have to revert this revision - it was breaking some objc stuff.

One of the failed tests looks like:

printf("%s", (i ? i : i));  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}

After the change the type of (?:) seems to be int / long here due to promotion happened. Is this intended behavior? I'm not familliar objc that much..

[asl]

Ok, the reason for objc tests to fail is that we're loosing typedefs when using the type from UsualArithmeticConversions().

Consider the following code:

typedef long NSInteger;

int printf(const char * restrict, ...);

void foo(void) {
  NSInteger i = 0;
  printf("%s", i ? i : i);
}

AST before was:

`-ConditionalOperator 0x7f9d1086e978 <col:16, col:24> 'NSInteger':'long'
   |-ImplicitCastExpr 0x7f9d1086e930 <col:16> 'NSInteger':'long' <LValueToRValue>
   | `-DeclRefExpr 0x7f9d1086e8b8 <col:16> 'NSInteger':'long' lvalue Var 0x7f9d1086e780 'i' 'NSInteger':'long'
   |-ImplicitCastExpr 0x7f9d1086e948 <col:20> 'NSInteger':'long' <LValueToRValue>
   | `-DeclRefExpr 0x7f9d1086e8e0 <col:20> 'NSInteger':'long' lvalue Var 0x7f9d1086e780 'i' 'NSInteger':'long'
   `-ImplicitCastExpr 0x7f9d1086e960 <col:24> 'NSInteger':'long' <LValueToRValue>
     `-DeclRefExpr 0x7f9d1086e908 <col:24> 'NSInteger':'long' lvalue Var 0x7f9d1086e780 'i' 'NSInteger':'long'

AST now is:

`-ConditionalOperator 0x7ff93a01c178 <col:16, col:24> 'long'
  |-ImplicitCastExpr 0x7ff93a01c130 <col:16> 'NSInteger':'long' <LValueToRValue>
  | `-DeclRefExpr 0x7ff93a01c0b8 <col:16> 'NSInteger':'long' lvalue Var 0x7ff93a01bf80 'i' 'NSInteger':'long'
  |-ImplicitCastExpr 0x7ff93a01c148 <col:20> 'NSInteger':'long' <LValueToRValue>
  | `-DeclRefExpr 0x7ff93a01c0e0 <col:20> 'NSInteger':'long' lvalue Var 0x7ff93a01bf80 'i' 'NSInteger':'long'
  `-ImplicitCastExpr 0x7ff93a01c160 <col:24> 'NSInteger':'long' <LValueToRValue>
    `-DeclRefExpr 0x7ff93a01c108 <col:24> 'NSInteger':'long' lvalue Var 0x7ff93a01bf80 'i' 'NSInteger':'long'

Note that the type of ConditionalOperator changed. Is this expected? The special (darwin) case which looks for typedef is inside CheckPrintfHandler::checkFormatExpr()

lib/Sema/SemaExpr.cpp

QualType Sema::CheckConditionalOperands(ExprResult &Cond, ExprResult &LHS,
   if (checkCondition(*this, Cond.get()))
     return QualType();
 
   // Now check the two expressions.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType())
     return CheckVectorOperands(LHS, RHS, QuestionLoc, /*isCompAssign*/false);
 
-  UsualArithmeticConversions(LHS, RHS);
+  QualType ResTy = UsualArithmeticConversions(LHS, RHS);
   if (LHS.isInvalid() || RHS.isInvalid())
     return QualType();
 
   QualType CondTy = Cond.get()->getType();
   QualType LHSTy = LHS.get()->getType();
   QualType RHSTy = RHS.get()->getType();
 
   // If the condition is a vector, and both operands are scalar,
   // attempt to implicity convert them to the vector type to act like the
   // built in select. (OpenCL v1.1 s6.3.i)
   if (getLangOpts().OpenCL && CondTy->isVectorType())
     if (checkConditionalConvertScalarsToVectors(*this, LHS, RHS, CondTy))
       return QualType();
   
   // If both operands have arithmetic type, do the usual arithmetic conversions
   // to find a common type: C99 6.5.15p3,5.
-  if (LHSTy->isArithmeticType() && RHSTy->isArithmeticType())
-    return LHS.get()->getType();
+  if (LHSTy->isArithmeticType() && RHSTy->isArithmeticType()) {
+    LHS = ImpCastExprToType(LHS.get(), ResTy, PrepareScalarCast(LHS, ResTy));
+    RHS = ImpCastExprToType(RHS.get(), ResTy, PrepareScalarCast(RHS, ResTy));
+
+    return ResTy;
+  }
 
   // If both operands are the same structure or union type, the result is that
   // type.
   if (const RecordType *LHSRT = LHSTy->getAs<RecordType>()) {    // C99 6.5.15p3
     if (const RecordType *RHSRT = RHSTy->getAs<RecordType>())
       if (LHSRT->getDecl() == RHSRT->getDecl())
         // "If both the operands have structure or union type, the result has
         // that type."  This implies that CV qualifiers are dropped.

lib/Sema/SemaExprCXX.cpp

QualType Sema::CXXCheckConditionalOperands(ExprResult &Cond, ExprResult &LHS,
   // Extension: conditional operator involving vector types.
   if (LTy->isVectorType() || RTy->isVectorType())
     return CheckVectorOperands(LHS, RHS, QuestionLoc, /*isCompAssign*/false);
 
   //   -- The second and third operands have arithmetic or enumeration type;
   //      the usual arithmetic conversions are performed to bring them to a
   //      common type, and the result is of that type.
   if (LTy->isArithmeticType() && RTy->isArithmeticType()) {
-    UsualArithmeticConversions(LHS, RHS);
+    QualType ResTy = UsualArithmeticConversions(LHS, RHS);
     if (LHS.isInvalid() || RHS.isInvalid())
       return QualType();
-    return LHS.get()->getType();
+
+    LHS = ImpCastExprToType(LHS.get(), ResTy, PrepareScalarCast(LHS, ResTy));
+    RHS = ImpCastExprToType(RHS.get(), ResTy, PrepareScalarCast(RHS, ResTy));
+
+    return ResTy;
   }
 
   //   -- The second and third operands have pointer type, or one has pointer
   //      type and the other is a null pointer constant, or both are null
   //      pointer constants, at least one of which is non-integral; pointer
   //      conversions and qualification conversions are performed to bring them
   //      to their composite pointer type. The result is of the composite
   //      pointer type.

test/CodeGen/complex.c

 }
 
 void t8() {
   __complex__ int *x = &(__complex__ int){1};
 }
 
 const _Complex double test9const = 0;
 _Complex double test9func() { return test9const; }
+
+// D6217
+void t9() {
+  // Check for proper type promotion of conditional expression
+  char c[(int)(sizeof(typeof((0 ? 2.0f : (_Complex double) 2.0f))) - sizeof(_Complex double))];
+  // Check for proper codegen
+  (0 ? 2.0f : (_Complex double) 2.0f);
+}
+