GLSL: Simplify constructor parsing
Constructor argument checking rules are reorganized to make them
easier to understand and constructor node creation is made simpler.
This removes usage of constructor op codes from ParseContext. This
paves the way for getting rid of constructor op codes entirely, which
will remove duplicate information from the AST and simplify lots of
code.
This refactoring will make adding arrays of arrays slightly easier.
BUG=angleproject:1490
TEST=angle_unittests
Change-Id: I4053afec55111b629353b4ff7cb0451c1ae3511c
Reviewed-on: https://chromium-review.googlesource.com/498767
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/compiler/translator/IntermNode.cpp b/src/compiler/translator/IntermNode.cpp
index 38638f9..cbdca76 100644
--- a/src/compiler/translator/IntermNode.cpp
+++ b/src/compiler/translator/IntermNode.cpp
@@ -304,10 +304,10 @@
}
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 @@
}
}
- return TIntermAggregate::CreateConstructor(constType, sh::TypeToConstructorOperator(type),
- arguments);
+ return TIntermAggregate::CreateConstructor(constType, arguments);
}
// static
@@ -777,9 +776,7 @@
}
}
-//
-// 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 @@
case EOpConstructStruct:
return true;
default:
+ // EOpConstruct is not supposed to be used in the AST.
+ ASSERT(mOp != EOpConstruct);
return false;
}
}
diff --git a/src/compiler/translator/IntermNode.h b/src/compiler/translator/IntermNode.h
index de7ccf1..ea3f097 100644
--- a/src/compiler/translator/IntermNode.h
+++ b/src/compiler/translator/IntermNode.h
@@ -604,7 +604,6 @@
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() {}
diff --git a/src/compiler/translator/Operator.h b/src/compiler/translator/Operator.h
index 83b957d..7ec29ed 100644
--- a/src/compiler/translator/Operator.h
+++ b/src/compiler/translator/Operator.h
@@ -205,6 +205,10 @@
// 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,
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index 7210de5..3641275 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -546,121 +546,22 @@
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 @@
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 @@
}
}
}
- 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 @@
}
}
}
+ 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 @@
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 @@
// 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 @@
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 @@
}
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);
}
}
diff --git a/src/compiler/translator/ParseContext.h b/src/compiler/translator/ParseContext.h
index 618a701..c479c2d 100644
--- a/src/compiler/translator/ParseContext.h
+++ b/src/compiler/translator/ParseContext.h
@@ -113,7 +113,6 @@
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 @@
TIntermNode *thisNode,
const TSourceLoc &loc);
TIntermTyped *addConstructor(TIntermSequence *arguments,
- TOperator op,
TType type,
const TSourceLoc &line);
TIntermTyped *addNonConstructorFunctionCall(TFunction *fnCall,
diff --git a/src/compiler/translator/RemoveDynamicIndexing.cpp b/src/compiler/translator/RemoveDynamicIndexing.cpp
index a8f6b51..5d136a3 100644
--- a/src/compiler/translator/RemoveDynamicIndexing.cpp
+++ b/src/compiler/translator/RemoveDynamicIndexing.cpp
@@ -110,7 +110,7 @@
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)
diff --git a/src/compiler/translator/RewriteTexelFetchOffset.cpp b/src/compiler/translator/RewriteTexelFetchOffset.cpp
index 554b044..13f834a 100644
--- a/src/compiler/translator/RewriteTexelFetchOffset.cpp
+++ b/src/compiler/translator/RewriteTexelFetchOffset.cpp
@@ -110,7 +110,7 @@
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());
}
diff --git a/src/compiler/translator/Types.cpp b/src/compiler/translator/Types.cpp
index fb286f1..2015b56 100644
--- a/src/compiler/translator/Types.cpp
+++ b/src/compiler/translator/Types.cpp
@@ -125,6 +125,8 @@
interfaceBlock(0),
structure(0)
{
+ ASSERT(primarySize <= 4);
+ ASSERT(secondarySize <= 4);
if (p.getUserDef())
structure = p.getUserDef()->getStruct();
}
@@ -134,6 +136,21 @@
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())
diff --git a/src/compiler/translator/Types.h b/src/compiler/translator/Types.h
index fac9dbb..721c090 100644
--- a/src/compiler/translator/Types.h
+++ b/src/compiler/translator/Types.h
@@ -318,6 +318,7 @@
{
if (primarySize != ps)
{
+ ASSERT(ps <= 4);
primarySize = ps;
invalidateMangledName();
}
@@ -326,6 +327,7 @@
{
if (secondarySize != ss)
{
+ ASSERT(ss <= 4);
secondarySize = ss;
invalidateMangledName();
}
@@ -377,6 +379,8 @@
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)
{
