Unify and simplify shader variable collection

src/compiler/translator/VariableInfo.cpp

@@ -448,79 +448,106 @@ void CollectVariables::visitSymbol(TIntermSymbol *symbol)
     }
 }
 
-class NameHashingTraverser : public GetVariableTraverser
+void CollectVariables::setCommonVariableProperties(const TType &type,
+                                                   const TString &name,
+                                                   ShaderVariable *variableOut) const
 {
-  public:
-    NameHashingTraverser(ShHashFunction64 hashFunction, const TSymbolTable &symbolTable)
-        : GetVariableTraverser(symbolTable), mHashFunction(hashFunction)
+    ASSERT(variableOut);
+
+    const TStructure *structure = type.getStruct();
+
+    if (!structure)
     {
+        variableOut->type      = GLVariableType(type);
+        variableOut->precision = GLVariablePrecision(type);
     }
+    else
+    {
+        // Note: this enum value is not exposed outside ANGLE
+        variableOut->type       = GL_STRUCT_ANGLEX;
+        variableOut->structName = structure->name().c_str();
+
+        const TFieldList &fields = structure->fields();
 
-  private:
-    void visitVariable(ShaderVariable *variable) override
+        for (TField *field : fields)
         {
-        TString stringName   = TString(variable->name.c_str());
-        variable->mappedName = TIntermTraverser::hash(stringName, mHashFunction).c_str();
+            // Regardless of the variable type (uniform, in/out etc.) its fields are always plain
+            // ShaderVariable objects.
+            ShaderVariable fieldVariable;
+            setCommonVariableProperties(*field->type(), field->name(), &fieldVariable);
+            variableOut->fields.push_back(fieldVariable);
         }
+    }
+    variableOut->name       = name.c_str();
+    variableOut->mappedName = TIntermTraverser::hash(name, mHashFunction).c_str();
+    variableOut->arraySize  = type.getArraySize();
+}
 
-    ShHashFunction64 mHashFunction;
-};
-
-// Attributes, which cannot have struct fields, are a special case
-template <>
-void CollectVariables::visitVariable(const TIntermSymbol *variable,
-                                     std::vector<Attribute> *infoList) const
+Attribute CollectVariables::recordAttribute(const TIntermSymbol &variable) const
 {
-    ASSERT(variable);
-    const TType &type = variable->getType();
+    const TType &type = variable.getType();
     ASSERT(!type.getStruct());
 
     Attribute attribute;
+    setCommonVariableProperties(type, variable.getSymbol(), &attribute);
 
-    attribute.type       = GLVariableType(type);
-    attribute.precision  = GLVariablePrecision(type);
-    attribute.name       = variable->getSymbol().c_str();
-    attribute.arraySize  = type.getArraySize();
-    attribute.mappedName = TIntermTraverser::hash(variable->getSymbol(), mHashFunction).c_str();
-    attribute.location   = variable->getType().getLayoutQualifier().location;
-
-    infoList->push_back(attribute);
+    attribute.location = type.getLayoutQualifier().location;
+    return attribute;
 }
 
-template <>
-void CollectVariables::visitVariable(const TIntermSymbol *variable,
-                                     std::vector<OutputVariable> *infoList) const
+OutputVariable CollectVariables::recordOutputVariable(const TIntermSymbol &variable) const
 {
-    ASSERT(variable);
-    const TType &type = variable->getType();
+    const TType &type = variable.getType();
     ASSERT(!type.getStruct());
 
-    OutputVariable attribute;
+    OutputVariable outputVariable;
+    setCommonVariableProperties(type, variable.getSymbol(), &outputVariable);
+
+    outputVariable.location = type.getLayoutQualifier().location;
+    return outputVariable;
+}
+
+Varying CollectVariables::recordVarying(const TIntermSymbol &variable) const
+{
+    const TType &type = variable.getType();
 
-    attribute.type       = GLVariableType(type);
-    attribute.precision  = GLVariablePrecision(type);
-    attribute.name       = variable->getSymbol().c_str();
-    attribute.arraySize  = type.getArraySize();
-    attribute.mappedName = TIntermTraverser::hash(variable->getSymbol(), mHashFunction).c_str();
-    attribute.location   = variable->getType().getLayoutQualifier().location;
+    Varying varying;
+    setCommonVariableProperties(type, variable.getSymbol(), &varying);
 
-    infoList->push_back(attribute);
+    switch (type.getQualifier())
+    {
+        case EvqVaryingIn:
+        case EvqVaryingOut:
+        case EvqVertexOut:
+        case EvqSmoothOut:
+        case EvqFlatOut:
+        case EvqCentroidOut:
+            if (mSymbolTable.isVaryingInvariant(std::string(variable.getSymbol().c_str())) ||
+                type.isInvariant())
+            {
+                varying.isInvariant = true;
+            }
+            break;
+        default:
+            break;
+    }
+
+    varying.interpolation = GetInterpolationType(type.getQualifier());
+    return varying;
 }
 
-template <>
-void CollectVariables::visitVariable(const TIntermSymbol *variable,
-                                     std::vector<InterfaceBlock> *infoList) const
+InterfaceBlock CollectVariables::recordInterfaceBlock(const TIntermSymbol &variable) const
 {
-    InterfaceBlock interfaceBlock;
-    const TInterfaceBlock *blockType = variable->getType().getInterfaceBlock();
+    const TInterfaceBlock *blockType = variable.getType().getInterfaceBlock();
     ASSERT(blockType);
 
+    InterfaceBlock interfaceBlock;
     interfaceBlock.name = blockType->name().c_str();
     interfaceBlock.mappedName =
         TIntermTraverser::hash(blockType->name().c_str(), mHashFunction).c_str();
     interfaceBlock.instanceName =
         (blockType->hasInstanceName() ? blockType->instanceName().c_str() : "");
-    interfaceBlock.arraySize        = variable->getArraySize();
+    interfaceBlock.arraySize        = variable.getArraySize();
     interfaceBlock.isRowMajorLayout = (blockType->matrixPacking() == EmpRowMajor);
     interfaceBlock.layout           = GetBlockLayoutType(blockType->blockStorage());
 
@@ -529,39 +556,20 @@ void CollectVariables::visitVariable(const TIntermSymbol *variable,
     {
         const TType &fieldType = *field->type();
 
-        NameHashingTraverser traverser(mHashFunction, mSymbolTable);
-        traverser.traverse(fieldType, field->name(), &interfaceBlock.fields);
-
-        interfaceBlock.fields.back().isRowMajorLayout =
+        InterfaceBlockField fieldVariable;
+        setCommonVariableProperties(fieldType, field->name(), &fieldVariable);
+        fieldVariable.isRowMajorLayout =
             (fieldType.getLayoutQualifier().matrixPacking == EmpRowMajor);
+        interfaceBlock.fields.push_back(fieldVariable);
     }
-
-    infoList->push_back(interfaceBlock);
+    return interfaceBlock;
 }
 
-template <typename VarT>
-void CollectVariables::visitVariable(const TIntermSymbol *variable,
-                                     std::vector<VarT> *infoList) const
+Uniform CollectVariables::recordUniform(const TIntermSymbol &variable) const
 {
-    NameHashingTraverser traverser(mHashFunction, mSymbolTable);
-    traverser.traverse(variable->getType(), variable->getSymbol(), infoList);
-}
-
-template <typename VarT>
-void CollectVariables::visitInfoList(const TIntermSequence &sequence,
-                                     std::vector<VarT> *infoList) const
-{
-    for (size_t seqIndex = 0; seqIndex < sequence.size(); seqIndex++)
-    {
-        const TIntermSymbol *variable = sequence[seqIndex]->getAsSymbolNode();
-        // The only case in which the sequence will not contain a
-        // TIntermSymbol node is initialization. It will contain a
-        // TInterBinary node in that case. Since attributes, uniforms,
-        // and varyings cannot be initialized in a shader, we must have
-        // only TIntermSymbol nodes in the sequence.
-        ASSERT(variable != NULL);
-        visitVariable(variable, infoList);
-    }
+    Uniform uniform;
+    setCommonVariableProperties(variable.getType(), variable.getSymbol(), &uniform);
+    return uniform;
 }
 
 bool CollectVariables::visitDeclaration(Visit, TIntermDeclaration *node)
@@ -572,35 +580,50 @@ bool CollectVariables::visitDeclaration(Visit, TIntermDeclaration *node)
     const TIntermTyped &typedNode = *(sequence.front()->getAsTyped());
     TQualifier qualifier          = typedNode.getQualifier();
 
+    bool isShaderVariable = qualifier == EvqAttribute || qualifier == EvqVertexIn ||
+                            qualifier == EvqFragmentOut || qualifier == EvqUniform ||
+                            IsVarying(qualifier);
+
+    if (typedNode.getBasicType() != EbtInterfaceBlock && !isShaderVariable)
+    {
+        return true;
+    }
+
+    for (TIntermNode *variableNode : sequence)
+    {
+        // The only case in which the sequence will not contain a TIntermSymbol node is
+        // initialization. It will contain a TInterBinary node in that case. Since attributes,
+        // uniforms, varyings, outputs and interface blocks cannot be initialized in a shader, we
+        // must have only TIntermSymbol nodes in the sequence in the cases we are interested in.
+        const TIntermSymbol &variable = *variableNode->getAsSymbolNode();
         if (typedNode.getBasicType() == EbtInterfaceBlock)
         {
-        visitInfoList(sequence, mInterfaceBlocks);
-        return false;
+            mInterfaceBlocks->push_back(recordInterfaceBlock(variable));
         }
-    else if (qualifier == EvqAttribute || qualifier == EvqVertexIn || qualifier == EvqFragmentOut ||
-             qualifier == EvqUniform || IsVarying(qualifier))
+        else
         {
             switch (qualifier)
             {
                 case EvqAttribute:
                 case EvqVertexIn:
-                visitInfoList(sequence, mAttribs);
+                    mAttribs->push_back(recordAttribute(variable));
                     break;
                 case EvqFragmentOut:
-                visitInfoList(sequence, mOutputVariables);
+                    mOutputVariables->push_back(recordOutputVariable(variable));
                     break;
                 case EvqUniform:
-                visitInfoList(sequence, mUniforms);
+                    mUniforms->push_back(recordUniform(variable));
                     break;
                 default:
-                visitInfoList(sequence, mVaryings);
+                    mVaryings->push_back(recordVarying(variable));
                     break;
             }
-
-        return false;
+        }
     }
 
-    return true;
+    // None of the recorded variables can have initializers, so we don't need to traverse the
+    // declarators.
+    return false;
 }
 
 bool CollectVariables::visitBinary(Visit, TIntermBinary *binaryNode)

src/compiler/translator/VariableInfo.h

@@ -35,11 +35,15 @@ class CollectVariables : public TIntermTraverser
     bool visitBinary(Visit visit, TIntermBinary *binaryNode) override;
 
   private:
-    template <typename VarT>
-    void visitVariable(const TIntermSymbol *variable, std::vector<VarT> *infoList) const;
-
-    template <typename VarT>
-    void visitInfoList(const TIntermSequence &sequence, std::vector<VarT> *infoList) const;
+    void setCommonVariableProperties(const TType &type,
+                                     const TString &name,
+                                     ShaderVariable *variableOut) const;
+
+    Attribute recordAttribute(const TIntermSymbol &variable) const;
+    OutputVariable recordOutputVariable(const TIntermSymbol &variable) const;
+    Varying recordVarying(const TIntermSymbol &variable) const;
+    InterfaceBlock recordInterfaceBlock(const TIntermSymbol &variable) const;
+    Uniform recordUniform(const TIntermSymbol &variable) const;
 
     std::vector<Attribute> *mAttribs;
     std::vector<OutputVariable> *mOutputVariables;

src/compiler/translator/util.cpp

@@ -701,97 +701,6 @@ TOperator TypeToConstructorOperator(const TType &type)
     return EOpNull;
 }
 
-GetVariableTraverser::GetVariableTraverser(const TSymbolTable &symbolTable)
-    : mSymbolTable(symbolTable)
-{
-}
-
-template void GetVariableTraverser::setTypeSpecificInfo(const TType &type,
-                                                        const TString &name,
-                                                        InterfaceBlockField *variable);
-template void GetVariableTraverser::setTypeSpecificInfo(const TType &type,
-                                                        const TString &name,
-                                                        ShaderVariable *variable);
-template void GetVariableTraverser::setTypeSpecificInfo(const TType &type,
-                                                        const TString &name,
-                                                        Uniform *variable);
-
-template <>
-void GetVariableTraverser::setTypeSpecificInfo(const TType &type,
-                                               const TString &name,
-                                               Varying *variable)
-{
-    ASSERT(variable);
-    switch (type.getQualifier())
-    {
-        case EvqVaryingIn:
-        case EvqVaryingOut:
-        case EvqVertexOut:
-        case EvqSmoothOut:
-        case EvqFlatOut:
-        case EvqCentroidOut:
-            if (mSymbolTable.isVaryingInvariant(std::string(name.c_str())) || type.isInvariant())
-            {
-                variable->isInvariant = true;
-            }
-            break;
-        default:
-            break;
-    }
-
-    variable->interpolation = GetInterpolationType(type.getQualifier());
-}
-
-template <typename VarT>
-void GetVariableTraverser::traverse(const TType &type,
-                                    const TString &name,
-                                    std::vector<VarT> *output)
-{
-    const TStructure *structure = type.getStruct();
-
-    VarT variable;
-    variable.name      = name.c_str();
-    variable.arraySize = type.getArraySize();
-
-    if (!structure)
-    {
-        variable.type      = GLVariableType(type);
-        variable.precision = GLVariablePrecision(type);
-    }
-    else
-    {
-        // Note: this enum value is not exposed outside ANGLE
-        variable.type       = GL_STRUCT_ANGLEX;
-        variable.structName = structure->name().c_str();
-
-        const TFieldList &fields = structure->fields();
-
-        for (size_t fieldIndex = 0; fieldIndex < fields.size(); fieldIndex++)
-        {
-            TField *field = fields[fieldIndex];
-            traverse(*field->type(), field->name(), &variable.fields);
-        }
-    }
-    setTypeSpecificInfo(type, name, &variable);
-    visitVariable(&variable);
-
-    ASSERT(output);
-    output->push_back(variable);
-}
-
-template void GetVariableTraverser::traverse(const TType &,
-                                             const TString &,
-                                             std::vector<InterfaceBlockField> *);
-template void GetVariableTraverser::traverse(const TType &,
-                                             const TString &,
-                                             std::vector<ShaderVariable> *);
-template void GetVariableTraverser::traverse(const TType &,
-                                             const TString &,
-                                             std::vector<Uniform> *);
-template void GetVariableTraverser::traverse(const TType &,
-                                             const TString &,
-                                             std::vector<Varying> *);
-
 // GLSL ES 1.0.17 4.6.1 The Invariant Qualifier
 bool CanBeInvariantESSL1(TQualifier qualifier)
 {

src/compiler/translator/util.h

@@ -45,30 +45,6 @@ TType GetShaderVariableBasicType(const sh::ShaderVariable &var);
 
 TOperator TypeToConstructorOperator(const TType &type);
 
-class GetVariableTraverser : angle::NonCopyable
-{
-  public:
-    GetVariableTraverser(const TSymbolTable &symbolTable);
-    virtual ~GetVariableTraverser() {}
-
-    template <typename VarT>
-    void traverse(const TType &type, const TString &name, std::vector<VarT> *output);
-
-  protected:
-    // May be overloaded
-    virtual void visitVariable(ShaderVariable *newVar) {}
-
-  private:
-    // Helper function called by traverse() to fill specific fields
-    // for attributes/varyings/uniforms.
-    template <typename VarT>
-    void setTypeSpecificInfo(const TType &type, const TString &name, VarT *variable)
-    {
-    }
-
-    const TSymbolTable &mSymbolTable;
-};
-
 bool IsBuiltinOutputVariable(TQualifier qualifier);
 bool IsBuiltinFragmentInputVariable(TQualifier qualifier);
 bool CanBeInvariantESSL1(TQualifier qualifier);

src/tests/compiler_tests/CollectVariables_test.cpp

@@ -740,3 +740,63 @@ TEST_F(CollectHashedVertexVariablesTest, StructUniform)
     EXPECT_EQ("webgl_5", field.mappedName);
     EXPECT_TRUE(field.fields.empty());
 }
+
+// Test a uniform declaration with multiple declarators.
+TEST_F(CollectFragmentVariablesTest, MultiDeclaration)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 out_fragColor;\n"
+        "uniform float uA, uB;\n"
+        "void main()\n"
+        "{\n"
+        "    vec4 color = vec4(uA, uA, uA, uB);\n"
+        "    out_fragColor = color;\n"
+        "}\n";
+
+    compile(shaderString);
+
+    const auto &uniforms = mTranslator->getUniforms();
+    ASSERT_EQ(2u, uniforms.size());
+
+    const Uniform &uniform = uniforms[0];
+    EXPECT_EQ(0u, uniform.arraySize);
+    EXPECT_GLENUM_EQ(GL_MEDIUM_FLOAT, uniform.precision);
+    EXPECT_TRUE(uniform.staticUse);
+    EXPECT_GLENUM_EQ(GL_FLOAT, uniform.type);
+    EXPECT_EQ("uA", uniform.name);
+
+    const Uniform &uniformB = uniforms[1];
+    EXPECT_EQ(0u, uniformB.arraySize);
+    EXPECT_GLENUM_EQ(GL_MEDIUM_FLOAT, uniformB.precision);
+    EXPECT_TRUE(uniformB.staticUse);
+    EXPECT_GLENUM_EQ(GL_FLOAT, uniformB.type);
+    EXPECT_EQ("uB", uniformB.name);
+}
+
+// Test a uniform declaration starting with an empty declarator.
+TEST_F(CollectFragmentVariablesTest, EmptyDeclarator)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 out_fragColor;\n"
+        "uniform float /* empty declarator */, uB;\n"
+        "void main()\n"
+        "{\n"
+        "    out_fragColor = vec4(uB, uB, uB, uB);\n"
+        "}\n";
+
+    compile(shaderString);
+
+    const auto &uniforms = mTranslator->getUniforms();
+    ASSERT_EQ(1u, uniforms.size());
+
+    const Uniform &uniformB = uniforms[0];
+    EXPECT_EQ(0u, uniformB.arraySize);
+    EXPECT_GLENUM_EQ(GL_MEDIUM_FLOAT, uniformB.precision);
+    EXPECT_TRUE(uniformB.staticUse);
+    EXPECT_GLENUM_EQ(GL_FLOAT, uniformB.type);
+    EXPECT_EQ("uB", uniformB.name);
+}