Refactor ternary operator parsing

src/compiler/translator/Intermediate.cpp

@@ -305,22 +305,17 @@ TIntermTyped *TIntermediate::addComma(
 // a true path, and a false path.  The two paths are specified
 // as separate parameters.
 //
-// Returns the selection node created, or 0 if one could not be.
+// Returns the selection node created, or one of trueBlock and falseBlock if the expression could be folded.
 //
-TIntermTyped *TIntermediate::addSelection(
-    TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock,
-    const TSourceLoc &line)
+TIntermTyped *TIntermediate::addSelection(TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock,
+                                          const TSourceLoc &line)
 {
-    if (!cond || !trueBlock || !falseBlock ||
-        trueBlock->getType() != falseBlock->getType())
-    {
-        return NULL;
-    }
-
-    //
-    // See if all the operands are constant, then fold it otherwise not.
-    //
-
+    // Right now it's safe to fold ternary operators only when all operands
+    // are constant. If only the condition is constant, it's theoretically
+    // possible to fold the ternary operator, but that requires making sure
+    // that the node returned from here won't be treated as a constant
+    // expression in case the node that gets eliminated was not a constant
+    // expression.
     if (cond->getAsConstantUnion() &&
         trueBlock->getAsConstantUnion() &&
         falseBlock->getAsConstantUnion())
@@ -334,8 +329,7 @@ TIntermTyped *TIntermediate::addSelection(
     //
     // Make a selection node.
     //
-    TIntermSelection *node = new TIntermSelection(
-        cond, trueBlock, falseBlock, trueBlock->getType());
+    TIntermSelection *node = new TIntermSelection(cond, trueBlock, falseBlock, trueBlock->getType());
     node->getTypePointer()->setQualifier(EvqTemporary);
     node->setLine(line);
 

src/compiler/translator/Intermediate.h

@@ -41,8 +41,8 @@ class TIntermediate
     TIntermAggregate *makeAggregate(TIntermNode *node, const TSourceLoc &);
     TIntermAggregate *setAggregateOperator(TIntermNode *, TOperator, const TSourceLoc &);
     TIntermNode *addSelection(TIntermTyped *cond, TIntermNodePair code, const TSourceLoc &);
-    TIntermTyped *addSelection(
-        TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock, const TSourceLoc &);
+    TIntermTyped *addSelection(TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock,
+                               const TSourceLoc &line);
     TIntermSwitch *addSwitch(
         TIntermTyped *init, TIntermAggregate *statementList, const TSourceLoc &line);
     TIntermCase *addCase(

src/compiler/translator/ParseContext.cpp

@@ -3258,6 +3258,20 @@ TIntermTyped *TParseContext::addFunctionCallOrMethod(TFunction *fnCall, TIntermN
     return callNode;
 }
 
+TIntermTyped *TParseContext::addTernarySelection(TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock,
+                                                 const TSourceLoc &loc)
+{
+    if (boolErrorCheck(loc, cond))
+        recover();
+
+    if (trueBlock->getType() != falseBlock->getType())
+    {
+        binaryOpError(loc, ":", trueBlock->getCompleteString(), falseBlock->getCompleteString());
+        recover();
+        return falseBlock;
+    }
+    return intermediate.addSelection(cond, trueBlock, falseBlock, loc);
+}
 
 //
 // Parse an array of strings using yyparse.

src/compiler/translator/ParseContext.h

@@ -212,6 +212,9 @@ struct TParseContext {
     TIntermTyped *addFunctionCallOrMethod(TFunction *fnCall, TIntermNode *node,
         const TSourceLoc &loc, bool *fatalError);
 
+    TIntermTyped *addTernarySelection(TIntermTyped *cond, TIntermTyped *trueBlock, TIntermTyped *falseBlock,
+                                      const TSourceLoc &line);
+
   private:
     bool declareVariable(const TSourceLoc &line, const TString &identifier, const TType &type, TVariable **variable);
 

src/compiler/translator/glslang.y

@@ -521,18 +521,7 @@ logical_or_expression
 conditional_expression
     : logical_or_expression { $$ = $1; }
     | logical_or_expression QUESTION expression COLON assignment_expression {
-       if (context->boolErrorCheck(@2, $1))
-            context->recover();
-
-        $$ = context->intermediate.addSelection($1, $3, $5, @2);
-        if ($3->getType() != $5->getType())
-            $$ = 0;
-
-        if ($$ == 0) {
-            context->binaryOpError(@2, ":", $3->getCompleteString(), $5->getCompleteString());
-            context->recover();
-            $$ = $5;
-        }
+        $$ = context->addTernarySelection($1, $3, $5, @2);
     }
     ;
 

src/compiler/translator/glslang_tab.cpp

@@ -698,27 +698,27 @@ static const yytype_uint16 yyrline[] =
      416,   419,   422,   429,   430,   433,   439,   440,   444,   451,
      452,   455,   458,   461,   467,   468,   471,   477,   478,   485,
      486,   493,   494,   501,   502,   508,   509,   515,   516,   522,
-     523,   540,   541,   549,   550,   551,   552,   556,   557,   558,
-     562,   566,   570,   574,   581,   584,   595,   603,   611,   639,
-     645,   656,   660,   664,   668,   675,   731,   734,   741,   749,
-     770,   791,   801,   829,   834,   844,   849,   859,   862,   865,
-     868,   874,   881,   884,   888,   892,   897,   902,   909,   913,
-     917,   921,   926,   931,   935,   942,   952,   958,   961,   967,
-     973,   980,   989,   998,  1006,  1009,  1016,  1020,  1027,  1030,
-    1034,  1038,  1047,  1056,  1064,  1074,  1086,  1089,  1092,  1098,
-    1105,  1108,  1114,  1117,  1120,  1126,  1129,  1134,  1149,  1153,
-    1157,  1161,  1165,  1169,  1174,  1179,  1184,  1189,  1194,  1199,
-    1204,  1209,  1214,  1219,  1224,  1229,  1234,  1239,  1244,  1249,
-    1254,  1259,  1264,  1269,  1274,  1278,  1282,  1286,  1290,  1294,
-    1298,  1302,  1306,  1310,  1314,  1318,  1322,  1326,  1330,  1334,
-    1342,  1350,  1354,  1367,  1367,  1370,  1370,  1376,  1379,  1395,
-    1398,  1407,  1411,  1417,  1424,  1439,  1443,  1447,  1448,  1454,
-    1455,  1456,  1457,  1458,  1459,  1460,  1464,  1465,  1465,  1465,
-    1475,  1476,  1480,  1480,  1481,  1481,  1486,  1489,  1499,  1502,
-    1508,  1509,  1513,  1521,  1525,  1532,  1532,  1539,  1542,  1549,
-    1554,  1569,  1569,  1574,  1574,  1581,  1581,  1589,  1592,  1598,
-    1601,  1607,  1611,  1618,  1621,  1624,  1627,  1630,  1639,  1643,
-    1650,  1653,  1659,  1659
+     523,   529,   530,   538,   539,   540,   541,   545,   546,   547,
+     551,   555,   559,   563,   570,   573,   584,   592,   600,   628,
+     634,   645,   649,   653,   657,   664,   720,   723,   730,   738,
+     759,   780,   790,   818,   823,   833,   838,   848,   851,   854,
+     857,   863,   870,   873,   877,   881,   886,   891,   898,   902,
+     906,   910,   915,   920,   924,   931,   941,   947,   950,   956,
+     962,   969,   978,   987,   995,   998,  1005,  1009,  1016,  1019,
+    1023,  1027,  1036,  1045,  1053,  1063,  1075,  1078,  1081,  1087,
+    1094,  1097,  1103,  1106,  1109,  1115,  1118,  1123,  1138,  1142,
+    1146,  1150,  1154,  1158,  1163,  1168,  1173,  1178,  1183,  1188,
+    1193,  1198,  1203,  1208,  1213,  1218,  1223,  1228,  1233,  1238,
+    1243,  1248,  1253,  1258,  1263,  1267,  1271,  1275,  1279,  1283,
+    1287,  1291,  1295,  1299,  1303,  1307,  1311,  1315,  1319,  1323,
+    1331,  1339,  1343,  1356,  1356,  1359,  1359,  1365,  1368,  1384,
+    1387,  1396,  1400,  1406,  1413,  1428,  1432,  1436,  1437,  1443,
+    1444,  1445,  1446,  1447,  1448,  1449,  1453,  1454,  1454,  1454,
+    1464,  1465,  1469,  1469,  1470,  1470,  1475,  1478,  1488,  1491,
+    1497,  1498,  1502,  1510,  1514,  1521,  1521,  1528,  1531,  1538,
+    1543,  1558,  1558,  1563,  1563,  1570,  1570,  1578,  1581,  1587,
+    1590,  1596,  1600,  1607,  1610,  1613,  1616,  1619,  1628,  1632,
+    1639,  1642,  1648,  1648
 };
 #endif
 
@@ -2916,18 +2916,7 @@ yyreduce:
   case 70:
 
     {
-       if (context->boolErrorCheck((yylsp[-3]), (yyvsp[-4].interm.intermTypedNode)))
-            context->recover();
-
-        (yyval.interm.intermTypedNode) = context->intermediate.addSelection((yyvsp[-4].interm.intermTypedNode), (yyvsp[-2].interm.intermTypedNode), (yyvsp[0].interm.intermTypedNode), (yylsp[-3]));
-        if ((yyvsp[-2].interm.intermTypedNode)->getType() != (yyvsp[0].interm.intermTypedNode)->getType())
-            (yyval.interm.intermTypedNode) = 0;
-
-        if ((yyval.interm.intermTypedNode) == 0) {
-            context->binaryOpError((yylsp[-3]), ":", (yyvsp[-2].interm.intermTypedNode)->getCompleteString(), (yyvsp[0].interm.intermTypedNode)->getCompleteString());
-            context->recover();
-            (yyval.interm.intermTypedNode) = (yyvsp[0].interm.intermTypedNode);
-        }
+        (yyval.interm.intermTypedNode) = context->addTernarySelection((yyvsp[-4].interm.intermTypedNode), (yyvsp[-2].interm.intermTypedNode), (yyvsp[0].interm.intermTypedNode), (yylsp[-3]));
     }
 
     break;

src/tests/compiler_tests/MalformedShader_test.cpp

@@ -364,3 +364,22 @@ TEST_F(MalformedShaderTest, UninitializedImplicitArraySize)
         FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
     }
 }
+
+// An operator can only form a constant expression if all the operands are constant expressions
+// - even operands of ternary operator that are never evaluated. (ESSL 3.00 section 4.3.3)
+TEST_F(MalformedShaderTest, TernaryOperatorNotConstantExpression)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 my_FragColor;\n"
+        "uniform bool u;\n"
+        "void main() {\n"
+        "   const bool a = true ? true : u;\n"
+        "   my_FragColor = vec4(1.0);\n"
+        "}\n";
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
+    }
+}