GLSL: Simplify constructor parsing

src/compiler/translator/IntermNode.cpp

@@ -304,10 +304,10 @@ TIntermAggregate *TIntermAggregate::CreateBuiltInFunctionCall(const TFunction &f
 }
 
 TIntermAggregate *TIntermAggregate::CreateConstructor(const TType &type,
-                                                      TOperator op,
                                                       TIntermSequence *arguments)
 {
-    TIntermAggregate *constructorNode = new TIntermAggregate(type, op, arguments);
+    TIntermAggregate *constructorNode =
+        new TIntermAggregate(type, sh::TypeToConstructorOperator(type), arguments);
     ASSERT(constructorNode->isConstructor());
     return constructorNode;
 }
@@ -630,8 +630,7 @@ TIntermTyped *TIntermTyped::CreateZero(const TType &type)
         }
     }
 
-    return TIntermAggregate::CreateConstructor(constType, sh::TypeToConstructorOperator(type),
-                                               arguments);
+    return TIntermAggregate::CreateConstructor(constType, arguments);
 }
 
 // static
@@ -777,9 +776,7 @@ bool TIntermOperator::isMultiplication() const
     }
 }
 
-//
-// returns true if the operator is for one of the constructors
-//
+// Returns true if the operator is for one of the constructors.
 bool TIntermOperator::isConstructor() const
 {
     switch (mOp)
@@ -812,6 +809,8 @@ bool TIntermOperator::isConstructor() const
         case EOpConstructStruct:
             return true;
         default:
+            // EOpConstruct is not supposed to be used in the AST.
+            ASSERT(mOp != EOpConstruct);
             return false;
     }
 }

src/compiler/translator/IntermNode.h

@@ -604,7 +604,6 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
     static TIntermAggregate *CreateBuiltInFunctionCall(const TFunction &func,
                                                        TIntermSequence *arguments);
     static TIntermAggregate *CreateConstructor(const TType &type,
-                                               TOperator op,
                                                TIntermSequence *arguments);
     static TIntermAggregate *Create(const TType &type, TOperator op, TIntermSequence *arguments);
     ~TIntermAggregate() {}

src/compiler/translator/Operator.h

@@ -205,6 +205,10 @@ enum TOperator
     // Constructors
     //
 
+    // Used by ParseContext to denote any constructor.
+    EOpConstruct,
+
+    // In the AST, constructors are stored using the below more specific op codes.
     EOpConstructInt,
     EOpConstructUInt,
     EOpConstructBool,

src/compiler/translator/ParseContext.cpp

@@ -546,121 +546,22 @@ bool TParseContext::checkIsNotReserved(const TSourceLoc &line, const TString &id
     return true;
 }
 
-// Make sure there is enough data provided to the constructor to build
-// something of the type of the constructor.  Also returns the type of
-// the constructor.
+// Make sure the argument types are correct for constructing a specific type.
 bool TParseContext::checkConstructorArguments(const TSourceLoc &line,
                                               const TIntermSequence *arguments,
-                                              TOperator op,
                                               const TType &type)
 {
-    bool constructingMatrix = false;
-    switch (op)
-    {
-        case EOpConstructMat2:
-        case EOpConstructMat2x3:
-        case EOpConstructMat2x4:
-        case EOpConstructMat3x2:
-        case EOpConstructMat3:
-        case EOpConstructMat3x4:
-        case EOpConstructMat4x2:
-        case EOpConstructMat4x3:
-        case EOpConstructMat4:
-            constructingMatrix = true;
-            break;
-        default:
-            break;
-    }
-
-    //
-    // Note: It's okay to have too many components available, but not okay to have unused
-    // arguments.  'full' will go to true when enough args have been seen.  If we loop
-    // again, there is an extra argument, so 'overfull' will become true.
-    //
-
-    size_t size         = 0;
-    bool full           = false;
-    bool overFull       = false;
-    bool matrixInMatrix = false;
-    bool arrayArg       = false;
-    for (TIntermNode *arg : *arguments)
-    {
-        const TIntermTyped *argTyped = arg->getAsTyped();
-        size += argTyped->getType().getObjectSize();
-
-        if (constructingMatrix && argTyped->getType().isMatrix())
-            matrixInMatrix = true;
-        if (full)
-            overFull = true;
-        if (op != EOpConstructStruct && !type.isArray() && size >= type.getObjectSize())
-            full = true;
-        if (argTyped->getType().isArray())
-            arrayArg = true;
-    }
-
-    if (type.isArray())
-    {
-        // The size of an unsized constructor should already have been determined.
-        ASSERT(!type.isUnsizedArray());
-        if (static_cast<size_t>(type.getArraySize()) != arguments->size())
-        {
-            error(line, "array constructor needs one argument per array element", "constructor");
-            return false;
-        }
-    }
-
-    if (arrayArg && op != EOpConstructStruct)
-    {
-        error(line, "constructing from a non-dereferenced array", "constructor");
-        return false;
-    }
-
-    if (matrixInMatrix && !type.isArray())
-    {
-        if (arguments->size() != 1)
-        {
-            error(line, "constructing matrix from matrix can only take one argument",
-                  "constructor");
-            return false;
-        }
-    }
-
-    if (overFull)
-    {
-        error(line, "too many arguments", "constructor");
-        return false;
-    }
-
-    if (op == EOpConstructStruct && !type.isArray() &&
-        type.getStruct()->fields().size() != arguments->size())
-    {
-        error(line,
-              "Number of constructor parameters does not match the number of structure fields",
-              "constructor");
-        return false;
-    }
-
-    if (!type.isMatrix() || !matrixInMatrix)
-    {
-        if ((op != EOpConstructStruct && size != 1 && size < type.getObjectSize()) ||
-            (op == EOpConstructStruct && size < type.getObjectSize()))
-        {
-            error(line, "not enough data provided for construction", "constructor");
-            return false;
-        }
-    }
-
     if (arguments->empty())
     {
         error(line, "constructor does not have any arguments", "constructor");
         return false;
     }
 
-    for (TIntermNode *const &argNode : *arguments)
+    for (TIntermNode *arg : *arguments)
     {
-        TIntermTyped *argTyped = argNode->getAsTyped();
+        const TIntermTyped *argTyped = arg->getAsTyped();
         ASSERT(argTyped != nullptr);
-        if (op != EOpConstructStruct && IsOpaqueType(argTyped->getBasicType()))
+        if (type.getBasicType() != EbtStruct && IsOpaqueType(argTyped->getBasicType()))
         {
             std::string reason("cannot convert a variable with type ");
             reason += getBasicString(argTyped->getBasicType());
@@ -676,15 +577,21 @@ bool TParseContext::checkConstructorArguments(const TSourceLoc &line,
 
     if (type.isArray())
     {
+        // The size of an unsized constructor should already have been determined.
+        ASSERT(!type.isUnsizedArray());
+        if (static_cast<size_t>(type.getArraySize()) != arguments->size())
+        {
+            error(line, "array constructor needs one argument per array element", "constructor");
+            return false;
+        }
         // GLSL ES 3.00 section 5.4.4: Each argument must be the same type as the element type of
         // the array.
         for (TIntermNode *const &argNode : *arguments)
         {
             const TType &argType = argNode->getAsTyped()->getType();
-            // It has already been checked that the argument is not an array, but we can arrive
-            // here due to prior error conditions.
             if (argType.isArray())
             {
+                error(line, "constructing from a non-dereferenced array", "constructor");
                 return false;
             }
             if (!argType.sameElementType(type))
@@ -694,9 +601,16 @@ bool TParseContext::checkConstructorArguments(const TSourceLoc &line,
             }
         }
     }
-    else if (op == EOpConstructStruct)
+    else if (type.getBasicType() == EbtStruct)
     {
         const TFieldList &fields = type.getStruct()->fields();
+        if (fields.size() != arguments->size())
+        {
+            error(line,
+                  "Number of constructor parameters does not match the number of structure fields",
+                  "constructor");
+            return false;
+        }
 
         for (size_t i = 0; i < fields.size(); i++)
         {
@@ -709,6 +623,67 @@ bool TParseContext::checkConstructorArguments(const TSourceLoc &line,
             }
         }
     }
+    else
+    {
+        // We're constructing a scalar, vector, or matrix.
+
+        // Note: It's okay to have too many components available, but not okay to have unused
+        // arguments. 'full' will go to true when enough args have been seen. If we loop again,
+        // there is an extra argument, so 'overFull' will become true.
+
+        size_t size    = 0;
+        bool full      = false;
+        bool overFull  = false;
+        bool matrixArg = false;
+        for (TIntermNode *arg : *arguments)
+        {
+            const TIntermTyped *argTyped = arg->getAsTyped();
+            ASSERT(argTyped != nullptr);
+
+            if (argTyped->getType().isArray())
+            {
+                error(line, "constructing from a non-dereferenced array", "constructor");
+                return false;
+            }
+            if (argTyped->getType().isMatrix())
+            {
+                matrixArg = true;
+            }
+
+            size += argTyped->getType().getObjectSize();
+            if (full)
+            {
+                overFull = true;
+            }
+            if (type.getBasicType() != EbtStruct && !type.isArray() && size >= type.getObjectSize())
+            {
+                full = true;
+            }
+        }
+
+        if (type.isMatrix() && matrixArg)
+        {
+            if (arguments->size() != 1)
+            {
+                error(line, "constructing matrix from matrix can only take one argument",
+                      "constructor");
+                return false;
+            }
+        }
+        else
+        {
+            if (size != 1 && size < type.getObjectSize())
+            {
+                error(line, "not enough data provided for construction", "constructor");
+                return false;
+            }
+            if (overFull)
+            {
+                error(line, "too many arguments", "constructor");
+                return false;
+            }
+        }
+    }
 
     return true;
 }
@@ -2698,34 +2673,23 @@ TFunction *TParseContext::parseFunctionHeader(const TPublicType &type,
     return new TFunction(name, new TType(type));
 }
 
-TFunction *TParseContext::addConstructorFunc(const TPublicType &publicTypeIn)
+TFunction *TParseContext::addConstructorFunc(const TPublicType &publicType)
 {
-    TPublicType publicType = publicTypeIn;
     if (publicType.isStructSpecifier())
     {
         error(publicType.getLine(), "constructor can't be a structure definition",
               getBasicString(publicType.getBasicType()));
     }
 
-    TOperator op = EOpNull;
-    if (publicType.getUserDef())
+    TType *type = new TType(publicType);
+    if (!type->canBeConstructed())
     {
-        op = EOpConstructStruct;
-    }
-    else
-    {
-        op = sh::TypeToConstructorOperator(TType(publicType));
-        if (op == EOpNull)
-        {
-            error(publicType.getLine(), "cannot construct this type",
-                  getBasicString(publicType.getBasicType()));
-            publicType.setBasicType(EbtFloat);
-            op = EOpConstructFloat;
-        }
+        error(publicType.getLine(), "cannot construct this type",
+              getBasicString(publicType.getBasicType()));
+        type->setBasicType(EbtFloat);
     }
 
-    const TType *type = new TType(publicType);
-    return new TFunction(nullptr, type, op);
+    return new TFunction(nullptr, type, EOpConstruct);
 }
 
 // This function is used to test for the correctness of the parameters passed to various constructor
@@ -2734,7 +2698,6 @@ TFunction *TParseContext::addConstructorFunc(const TPublicType &publicTypeIn)
 // Returns a node to add to the tree regardless of if an error was generated or not.
 //
 TIntermTyped *TParseContext::addConstructor(TIntermSequence *arguments,
-                                            TOperator op,
                                             TType type,
                                             const TSourceLoc &line)
 {
@@ -2749,12 +2712,12 @@ TIntermTyped *TParseContext::addConstructor(TIntermSequence *arguments,
         type.setArraySize(static_cast<unsigned int>(arguments->size()));
     }
 
-    if (!checkConstructorArguments(line, arguments, op, type))
+    if (!checkConstructorArguments(line, arguments, type))
     {
         return TIntermTyped::CreateZero(type);
     }
 
-    TIntermAggregate *constructorNode = TIntermAggregate::CreateConstructor(type, op, arguments);
+    TIntermAggregate *constructorNode = TIntermAggregate::CreateConstructor(type, arguments);
     constructorNode->setLine(line);
 
     TIntermTyped *constConstructor =
@@ -4383,12 +4346,13 @@ TIntermTyped *TParseContext::addFunctionCallOrMethod(TFunction *fnCall,
     }
 
     TOperator op = fnCall->getBuiltInOp();
-    if (op != EOpNull)
+    if (op == EOpConstruct)
     {
-        return addConstructor(arguments, op, fnCall->getReturnType(), loc);
+        return addConstructor(arguments, fnCall->getReturnType(), loc);
     }
     else
     {
+        ASSERT(op == EOpNull);
         return addNonConstructorFunctionCall(fnCall, arguments, loc);
     }
 }

src/compiler/translator/ParseContext.h

@@ -113,7 +113,6 @@ class TParseContext : angle::NonCopyable
     bool checkIsAtGlobalLevel(const TSourceLoc &line, const char *token);
     bool checkConstructorArguments(const TSourceLoc &line,
                                    const TIntermSequence *arguments,
-                                   TOperator op,
                                    const TType &type);
 
     // Returns a sanitized array size to use (the size is at least 1).
@@ -415,7 +414,6 @@ class TParseContext : angle::NonCopyable
                             TIntermNode *thisNode,
                             const TSourceLoc &loc);
     TIntermTyped *addConstructor(TIntermSequence *arguments,
-                                 TOperator op,
                                  TType type,
                                  const TSourceLoc &line);
     TIntermTyped *addNonConstructorFunctionCall(TFunction *fnCall,

src/compiler/translator/RemoveDynamicIndexing.cpp

@@ -110,7 +110,7 @@ TIntermTyped *EnsureSignedInt(TIntermTyped *node)
 
     TIntermSequence *arguments = new TIntermSequence();
     arguments->push_back(node);
-    return TIntermAggregate::CreateConstructor(TType(EbtInt), EOpConstructInt, arguments);
+    return TIntermAggregate::CreateConstructor(TType(EbtInt), arguments);
 }
 
 TType GetFieldType(const TType &indexedType)

src/compiler/translator/RewriteTexelFetchOffset.cpp

@@ -110,7 +110,7 @@ bool Traverser::visitAggregate(Visit visit, TIntermAggregate *node)
         TIntermTyped *zeroNode = TIntermTyped::CreateZero(TType(EbtInt));
         constructOffsetIvecArguments->push_back(zeroNode);
 
-        offsetNode = TIntermAggregate::CreateConstructor(texCoordNode->getType(), EOpConstructIVec3,
+        offsetNode = TIntermAggregate::CreateConstructor(texCoordNode->getType(),
                                                          constructOffsetIvecArguments);
         offsetNode->setLine(texCoordNode->getLine());
     }

src/compiler/translator/Types.cpp

@@ -125,6 +125,8 @@ TType::TType(const TPublicType &p)
       interfaceBlock(0),
       structure(0)
 {
+    ASSERT(primarySize <= 4);
+    ASSERT(secondarySize <= 4);
     if (p.getUserDef())
         structure = p.getUserDef()->getStruct();
 }
@@ -134,6 +136,21 @@ bool TStructure::equals(const TStructure &other) const
     return (uniqueId() == other.uniqueId());
 }
 
+bool TType::canBeConstructed() const
+{
+    switch (type)
+    {
+        case EbtFloat:
+        case EbtInt:
+        case EbtUInt:
+        case EbtBool:
+        case EbtStruct:
+            return true;
+        default:
+            return false;
+    }
+}
+
 const char *TType::getBuiltInTypeNameString() const
 {
     if (isMatrix())

src/compiler/translator/Types.h

@@ -318,6 +318,7 @@ class TType
     {
         if (primarySize != ps)
         {
+            ASSERT(ps <= 4);
             primarySize = ps;
             invalidateMangledName();
         }
@@ -326,6 +327,7 @@ class TType
     {
         if (secondarySize != ss)
         {
+            ASSERT(ss <= 4);
             secondarySize = ss;
             invalidateMangledName();
         }
@@ -377,6 +379,8 @@ class TType
     bool isScalarFloat() const { return isScalar() && type == EbtFloat; }
     bool isScalarInt() const { return isScalar() && (type == EbtInt || type == EbtUInt); }
 
+    bool canBeConstructed() const;
+
     TStructure *getStruct() const { return structure; }
     void setStruct(TStructure *s)
     {