commit 15200047d2d511f8bf69a09d05e8c87d94b7ebf2
author Olli Etuaho <oetuaho@nvidia.com> Wed Nov 04 16:56:31 2015 +0200
committer Olli Etuaho <oetuaho@nvidia.com> Fri Nov 06 08:58:50 2015 +0000
tree 75f2b401cc155cb31080adde7d2fcefc2e395f14
parent 0b2d2dcf95932306722aaffb227dfcc6671d0b2f

Fix issues in comma operator parsing

Always qualify comma operator result with EvqTemporary in ESSL3, as
specified.

Also, it is possible that in the future some expressions are qualified
as EvqConst but they'd still have side effects, in which case discarding
them when they're the left operand of the comma operator would be wrong.
This would be the case if ANGLE allowed "(a = b).length()" for example.
For this reason it is better to check whether the left node has side
effects, rather than check its const qualification, and only discard it
if it doesn't.

Also, Intermediate::addComma() never returns null, so there's no need to
check the result.

BUG=angleproject:1201
TEST=WebGL conformance tests
     conformance/glsl/misc/sequence-operator-returns-constant.html
     conformance2/glsl3/sequence-operator-returns-non-constant.html

Change-Id: Ibfbd92baa4910b14c0dc8f8a3c3008440d191cd6
Reviewed-on: https://chromium-review.googlesource.com/311171
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Tryjob-Request: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Tested-by: Olli Etuaho <oetuaho@nvidia.com>

src/compiler/translator/Intermediate.cpp

diff --git a/src/compiler/translator/Intermediate.cpp b/src/compiler/translator/Intermediate.cpp
index 62c6f13..931a4d9 100644
--- a/src/compiler/translator/Intermediate.cpp
+++ b/src/compiler/translator/Intermediate.cpp
@@ -281,22 +281,32 @@
     return node;
 }
 
-TIntermTyped *TIntermediate::addComma(
-    TIntermTyped *left, TIntermTyped *right, const TSourceLoc &line)
+TIntermTyped *TIntermediate::addComma(TIntermTyped *left,
+                                      TIntermTyped *right,
+                                      const TSourceLoc &line,
+                                      int shaderVersion)
 {
-    if (left->getType().getQualifier() == EvqConst &&
-        right->getType().getQualifier() == EvqConst)
+    TQualifier resultQualifier = EvqConst;
+    // ESSL3.00 section 12.43: The result of a sequence operator is not a constant-expression.
+    if (shaderVersion >= 300 || left->getQualifier() != EvqConst ||
+        right->getQualifier() != EvqConst)
     {
-        return right;
+        resultQualifier = EvqTemporary;
+    }
+
+    TIntermTyped *commaNode = nullptr;
+    if (!left->hasSideEffects())
+    {
+        commaNode = right;
     }
     else
     {
-        TIntermTyped *commaAggregate = growAggregate(left, right, line);
-        commaAggregate->getAsAggregate()->setOp(EOpComma);
-        commaAggregate->setType(right->getType());
-        commaAggregate->getTypePointer()->setQualifier(EvqTemporary);
-        return commaAggregate;
+        commaNode = growAggregate(left, right, line);
+        commaNode->getAsAggregate()->setOp(EOpComma);
+        commaNode->setType(right->getType());
     }
+    commaNode->getTypePointer()->setQualifier(resultQualifier);
+    return commaNode;
 }
 
 //

src/compiler/translator/Intermediate.h

diff --git a/src/compiler/translator/Intermediate.h b/src/compiler/translator/Intermediate.h
index 21d0b1d..7931424 100644
--- a/src/compiler/translator/Intermediate.h
+++ b/src/compiler/translator/Intermediate.h
@@ -48,8 +48,10 @@
         TIntermTyped *init, TIntermAggregate *statementList, const TSourceLoc &line);
     TIntermCase *addCase(
         TIntermTyped *condition, const TSourceLoc &line);
-    TIntermTyped *addComma(
-        TIntermTyped *left, TIntermTyped *right, const TSourceLoc &);
+    TIntermTyped *addComma(TIntermTyped *left,
+                           TIntermTyped *right,
+                           const TSourceLoc &line,
+                           int shaderVersion);
     TIntermConstantUnion *addConstantUnion(
         TConstantUnion *constantUnion, const TType &type, const TSourceLoc &line);
     // TODO(zmo): Get rid of default value.

src/compiler/translator/ParseContext.cpp

diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index 8e145c6..8d05d1e 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -3750,14 +3750,7 @@
                                       TIntermTyped *right,
                                       const TSourceLoc &loc)
 {
-    TIntermTyped *node = intermediate.addComma(left, right, loc);
-    if (node == nullptr)
-    {
-        binaryOpError(loc, ",", left->getCompleteString(), right->getCompleteString());
-        recover();
-        return right;
-    }
-    return node;
+    return intermediate.addComma(left, right, loc, mShaderVersion);
 }
 
 TIntermBranch *TParseContext::addBranch(TOperator op, const TSourceLoc &loc)