Clean up qualification order checks

src/compiler/translator/ParseContext.cpp

@@ -16,17 +16,6 @@
 #include "compiler/translator/ValidateGlobalInitializer.h"
 #include "compiler/translator/util.h"
 
-namespace
-{
-// GLSL ES 3.10 does not impose a strict order on type qualifiers and allows multiple layout
-// declarations
-// GLSL ES 3.10 Revision 4, 4.10 Order of Qualification
-bool AreTypeQualifierChecksRelaxed(int shaderVersion)
-{
-    return shaderVersion >= 310;
-}
-}  // namespace
-
 ///////////////////////////////////////////////////////////////////////
 //
 // Sub- vector and matrix fields
@@ -945,8 +934,7 @@ void TParseContext::checkIsParameterQualifierValid(
     const TTypeQualifierBuilder &typeQualifierBuilder,
     TType *type)
 {
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getParameterTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getParameterTypeQualifier(&mDiagnostics);
 
     if (typeQualifier.qualifier == EvqOut || typeQualifier.qualifier == EvqInOut)
     {
@@ -1414,8 +1402,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
 TPublicType TParseContext::addFullySpecifiedType(const TTypeQualifierBuilder &typeQualifierBuilder,
                                                  const TPublicType &typeSpecifier)
 {
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(&mDiagnostics);
 
     TPublicType returnType     = typeSpecifier;
     returnType.qualifier       = typeQualifier.qualifier;
@@ -1717,8 +1704,7 @@ TIntermAggregate *TParseContext::parseInvariantDeclaration(
     const TString *identifier,
     const TSymbol *symbol)
 {
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(&mDiagnostics);
 
     if (!typeQualifier.invariant)
     {
@@ -1926,8 +1912,7 @@ TIntermAggregate *TParseContext::parseArrayInitDeclarator(const TPublicType &pub
 
 void TParseContext::parseGlobalLayoutQualifier(const TTypeQualifierBuilder &typeQualifierBuilder)
 {
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(&mDiagnostics);
     const TLayoutQualifier layoutQualifier = typeQualifier.layoutQualifier;
 
     checkInvariantVariableQualifier(typeQualifier.invariant, typeQualifier.qualifier,
@@ -2507,8 +2492,7 @@ TIntermAggregate *TParseContext::addInterfaceBlock(
 {
     checkIsNotReserved(nameLine, blockName);
 
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(&mDiagnostics);
 
     if (typeQualifier.qualifier != EvqUniform)
     {
@@ -3191,6 +3175,13 @@ TLayoutQualifier TParseContext::parseLayoutQualifier(const TString &qualifierTyp
     return qualifier;
 }
 
+TTypeQualifierBuilder *TParseContext::createTypeQualifierBuilder(const TSourceLoc &loc)
+{
+    return new TTypeQualifierBuilder(
+        new TStorageQualifierWrapper(symbolTable.atGlobalLevel() ? EvqGlobal : EvqTemporary, loc),
+        mShaderVersion);
+}
+
 TLayoutQualifier TParseContext::joinLayoutQualifiers(TLayoutQualifier leftQualifier,
                                                      TLayoutQualifier rightQualifier,
                                                      const TSourceLoc &rightQualifierLocation)
@@ -3204,8 +3195,7 @@ TFieldList *TParseContext::addStructDeclaratorListWithQualifiers(
     TPublicType *typeSpecifier,
     TFieldList *fieldList)
 {
-    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(
-        &mDiagnostics, AreTypeQualifierChecksRelaxed(mShaderVersion));
+    TTypeQualifier typeQualifier = typeQualifierBuilder.getVariableTypeQualifier(&mDiagnostics);
 
     typeSpecifier->qualifier       = typeQualifier.qualifier;
     typeSpecifier->layoutQualifier = typeQualifier.layoutQualifier;

src/compiler/translator/ParseContext.h

@@ -327,6 +327,7 @@ class TParseContext : angle::NonCopyable
                                           const TSourceLoc &qualifierTypeLine,
                                           int intValue,
                                           const TSourceLoc &intValueLine);
+    TTypeQualifierBuilder *createTypeQualifierBuilder(const TSourceLoc &loc);
     TLayoutQualifier joinLayoutQualifiers(TLayoutQualifier leftQualifier,
                                           TLayoutQualifier rightQualifier,
                                           const TSourceLoc &rightQualifierLocation);

src/compiler/translator/QualifierTypes.cpp

@@ -54,6 +54,14 @@ TLayoutQualifier JoinLayoutQualifiers(TLayoutQualifier leftQualifier,
 namespace
 {
 
+// GLSL ES 3.10 does not impose a strict order on type qualifiers and allows multiple layout
+// declarations.
+// GLSL ES 3.10 Revision 4, 4.10 Order of Qualification
+bool AreTypeQualifierChecksRelaxed(int shaderVersion)
+{
+    return shaderVersion >= 310;
+}
+
 #if defined(ANGLE_ENABLE_ASSERTS)
 bool IsScopeQualifier(TQualifier qualifier)
 {
@@ -525,7 +533,9 @@ TTypeQualifier::TTypeQualifier(TQualifier scope, const TSourceLoc &loc)
     ASSERT(IsScopeQualifier(qualifier));
 }
 
-TTypeQualifierBuilder::TTypeQualifierBuilder(const TStorageQualifierWrapper *scope)
+TTypeQualifierBuilder::TTypeQualifierBuilder(const TStorageQualifierWrapper *scope,
+                                             int shaderVersion)
+    : mShaderVersion(shaderVersion)
 {
     ASSERT(IsScopeQualifier(scope->getQualifier()));
     mQualifiers.push_back(scope);
@@ -536,9 +546,9 @@ void TTypeQualifierBuilder::appendQualifier(const TQualifierWrapperBase *qualifi
     mQualifiers.push_back(qualifier);
 }
 
-bool TTypeQualifierBuilder::checkSequenceIsValid(TDiagnostics *diagnostics,
-                                                 bool areQualifierChecksRelaxed) const
+bool TTypeQualifierBuilder::checkSequenceIsValid(TDiagnostics *diagnostics) const
 {
+    bool areQualifierChecksRelaxed = AreTypeQualifierChecksRelaxed(mShaderVersion);
     std::string errorMessage;
     if (HasRepeatingQualifiers(mQualifiers, areQualifierChecksRelaxed, &errorMessage))
     {
@@ -557,15 +567,13 @@ bool TTypeQualifierBuilder::checkSequenceIsValid(TDiagnostics *diagnostics,
     return true;
 }
 
-TTypeQualifier TTypeQualifierBuilder::getParameterTypeQualifier(
-    TDiagnostics *diagnostics,
-    bool areQualifierChecksRelaxed) const
+TTypeQualifier TTypeQualifierBuilder::getParameterTypeQualifier(TDiagnostics *diagnostics) const
 {
     ASSERT(IsInvariantCorrect(mQualifiers));
     ASSERT(static_cast<const TStorageQualifierWrapper *>(mQualifiers[0])->getQualifier() ==
            EvqTemporary);
 
-    if (!checkSequenceIsValid(diagnostics, areQualifierChecksRelaxed))
+    if (!checkSequenceIsValid(diagnostics))
     {
         return TTypeQualifier(EvqTemporary, mQualifiers[0]->getLine());
     }
@@ -573,7 +581,7 @@ TTypeQualifier TTypeQualifierBuilder::getParameterTypeQualifier(
     // If the qualifier checks are relaxed, then it is easier to sort the qualifiers so
     // that the order imposed by the GLSL ES 3.00 spec is kept. Then we can use the same code to
     // combine the qualifiers.
-    if (areQualifierChecksRelaxed)
+    if (AreTypeQualifierChecksRelaxed(mShaderVersion))
     {
         // Copy the qualifier sequence so that we can sort them.
         QualifierSequence sortedQualifierSequence = mQualifiers;
@@ -583,12 +591,11 @@ TTypeQualifier TTypeQualifierBuilder::getParameterTypeQualifier(
     return GetParameterTypeQualifierFromSortedSequence(mQualifiers, diagnostics);
 }
 
-TTypeQualifier TTypeQualifierBuilder::getVariableTypeQualifier(TDiagnostics *diagnostics,
-                                                               bool areQualifierChecksRelaxed) const
+TTypeQualifier TTypeQualifierBuilder::getVariableTypeQualifier(TDiagnostics *diagnostics) const
 {
     ASSERT(IsInvariantCorrect(mQualifiers));
 
-    if (!checkSequenceIsValid(diagnostics, areQualifierChecksRelaxed))
+    if (!checkSequenceIsValid(diagnostics))
     {
         return TTypeQualifier(
             static_cast<const TStorageQualifierWrapper *>(mQualifiers[0])->getQualifier(),
@@ -598,7 +605,7 @@ TTypeQualifier TTypeQualifierBuilder::getVariableTypeQualifier(TDiagnostics *dia
     // If the qualifier checks are relaxed, then it is easier to sort the qualifiers so
     // that the order imposed by the GLSL ES 3.00 spec is kept. Then we can use the same code to
     // combine the qualifiers.
-    if (areQualifierChecksRelaxed)
+    if (AreTypeQualifierChecksRelaxed(mShaderVersion))
     {
         // Copy the qualifier sequence so that we can sort them.
         QualifierSequence sortedQualifierSequence = mQualifiers;

src/compiler/translator/QualifierTypes.h

@@ -150,23 +150,22 @@ class TTypeQualifierBuilder : angle::NonCopyable
     using QualifierSequence = std::vector<const TQualifierWrapperBase *>;
 
   public:
-    TTypeQualifierBuilder(const TStorageQualifierWrapper *scope);
+    TTypeQualifierBuilder(const TStorageQualifierWrapper *scope, int shaderVersion);
     // Adds the passed qualifier to the end of the sequence.
     void appendQualifier(const TQualifierWrapperBase *qualifier);
     // Checks for the order of qualification and repeating qualifiers.
-    bool checkSequenceIsValid(TDiagnostics *diagnostics, bool areQualifierChecksRelaxed) const;
+    bool checkSequenceIsValid(TDiagnostics *diagnostics) const;
     // Goes over the qualifier sequence and parses it to form a type qualifier for a function
     // parameter.
     // The returned object is initialized even if the parsing fails.
-    TTypeQualifier getParameterTypeQualifier(TDiagnostics *diagnostics,
-                                             bool areQualifierChecksRelaxed) const;
+    TTypeQualifier getParameterTypeQualifier(TDiagnostics *diagnostics) const;
     // Goes over the qualifier sequence and parses it to form a type qualifier for a variable.
     // The returned object is initialized even if the parsing fails.
-    TTypeQualifier getVariableTypeQualifier(TDiagnostics *diagnostics,
-                                            bool areQualifierChecksRelaxed) const;
+    TTypeQualifier getVariableTypeQualifier(TDiagnostics *diagnostics) const;
 
   private:
     QualifierSequence mQualifiers;
+    int mShaderVersion;
 };
 
 #endif  // COMPILER_TRANSLATOR_QUALIFIER_TYPES_H_

src/compiler/translator/glslang.y

@@ -836,7 +836,7 @@ interpolation_qualifier
 
 type_qualifier
     : single_type_qualifier {
-        $$ = new TTypeQualifierBuilder(new TStorageQualifierWrapper(context->symbolTable.atGlobalLevel() ? EvqGlobal : EvqTemporary, @1));
+        $$ = context->createTypeQualifierBuilder(@1);
         $$->appendQualifier($1);
     }
     | type_qualifier single_type_qualifier {

src/compiler/translator/glslang_tab.cpp

@@ -3414,7 +3414,7 @@ yyreduce:
   case 125:
 
     {
-        (yyval.interm.typeQualifierBuilder) = new TTypeQualifierBuilder(new TStorageQualifierWrapper(context->symbolTable.atGlobalLevel() ? EvqGlobal : EvqTemporary, (yylsp[0])));
+        (yyval.interm.typeQualifierBuilder) = context->createTypeQualifierBuilder((yylsp[0]));
         (yyval.interm.typeQualifierBuilder)->appendQualifier((yyvsp[0].interm.qualifierWrapper));
     }
 

src/tests/compiler_tests/QualificationOrderESSL31_test.cpp

@@ -41,11 +41,9 @@ class QualificationVertexShaderTestESSL31 : public testing::Test
         return mASTRoot != nullptr;
     }
 
-    const TIntermSymbol *findSymbolInAST(const TString &stringToFind, TBasicType basicType)
+    const TIntermSymbol *findSymbolInAST(const TString &symbolName, TBasicType basicType)
     {
-        ShaderVariableFinder finder(stringToFind, basicType);
-        mASTRoot->traverse(&finder);
-        return finder.getNode();
+        return FindSymbolNode(mASTRoot, symbolName, basicType);
     }
 
   protected:

src/tests/test_utils/compiler_test.cpp

@@ -11,6 +11,39 @@
 #include "angle_gl.h"
 #include "compiler/translator/Compiler.h"
 
+namespace
+{
+
+class ShaderVariableFinder : public TIntermTraverser
+{
+  public:
+    ShaderVariableFinder(const TString &variableName, TBasicType basicType)
+        : TIntermTraverser(true, false, false),
+          mVariableName(variableName),
+          mNodeFound(nullptr),
+          mBasicType(basicType)
+    {
+    }
+
+    void visitSymbol(TIntermSymbol *node)
+    {
+        if (node->getBasicType() == mBasicType && node->getSymbol() == mVariableName)
+        {
+            mNodeFound = node;
+        }
+    }
+
+    bool isFound() const { return mNodeFound != nullptr; }
+    const TIntermSymbol *getNode() const { return mNodeFound; }
+
+  private:
+    TString mVariableName;
+    TIntermSymbol *mNodeFound;
+    TBasicType mBasicType;
+};
+
+}  // anonymous namespace
+
 bool compileTestShader(GLenum type,
                        ShShaderSpec spec,
                        ShShaderOutput output,
@@ -172,3 +205,12 @@ bool MatchOutputCodeTest::notFoundInCode(const char *stringToFind) const
     }
     return true;
 }
+
+const TIntermSymbol *FindSymbolNode(TIntermNode *root,
+                                    const TString &symbolName,
+                                    TBasicType basicType)
+{
+    ShaderVariableFinder finder(symbolName, basicType);
+    root->traverse(&finder);
+    return finder.getNode();
+}

src/tests/test_utils/compiler_test.h

@@ -87,32 +87,8 @@ class MatchOutputCodeTest : public testing::Test
     std::map<ShShaderOutput, std::string> mOutputCode;
 };
 
-class ShaderVariableFinder : public TIntermTraverser
-{
-  public:
-    ShaderVariableFinder(const TString &variableName, TBasicType basicType)
-        : TIntermTraverser(true, false, false),
-          mVariableName(variableName),
-          mNodeFound(nullptr),
-          mBasicType(basicType)
-    {
-    }
-
-    void visitSymbol(TIntermSymbol *node)
-    {
-        if (node->getBasicType() == mBasicType && node->getSymbol() == mVariableName)
-        {
-            mNodeFound = node;
-        }
-    }
-
-    bool isFound() const { return mNodeFound != nullptr; }
-    const TIntermSymbol *getNode() const { return mNodeFound; }
-
-  private:
-    TString mVariableName;
-    TIntermSymbol *mNodeFound;
-    TBasicType mBasicType;
-};
+const TIntermSymbol *FindSymbolNode(TIntermNode *root,
+                                    const TString &symbolName,
+                                    TBasicType basicType);
 
 #endif // TESTS_TEST_UTILS_COMPILER_TEST_H_