Store shader interface variables as per query spec
GLES 3.1 section 7.3.1.1 specifies how active variable entries should
be generated and how active variables are named. The specs for program
interface variable queries are built on top of this section.
ANGLE has already followed this spec for the most part for generating
variable lists in ProgramState, but now we also follow the naming spec
for arrays and include [0] at the end of the stored name.
This will make implementing arrays of arrays more straightforward.
Most logic for variable queries will just keep working as is when
arrays of arrays are added instead of needing more complex logic for
handling array indexing.
BUG=angleproject:2125
TEST=angle_end2end_tests
Change-Id: I3acd14253153e10bc312114b0303065da2efb506
Reviewed-on: https://chromium-review.googlesource.com/739826
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
diff --git a/src/common/string_utils.cpp b/src/common/string_utils.cpp
index f7564db..26f384b 100644
--- a/src/common/string_utils.cpp
+++ b/src/common/string_utils.cpp
@@ -158,6 +158,11 @@
return Optional<std::vector<wchar_t>>(wcstring);
}
+bool BeginsWith(const std::string &str, const std::string &prefix)
+{
+ return strncmp(str.c_str(), prefix.c_str(), prefix.length()) == 0;
+}
+
bool BeginsWith(const std::string &str, const char *prefix)
{
return strncmp(str.c_str(), prefix, strlen(prefix)) == 0;
@@ -168,6 +173,11 @@
return strncmp(str, prefix, strlen(prefix)) == 0;
}
+bool BeginsWith(const std::string &str, const std::string &prefix, const size_t prefixLength)
+{
+ return strncmp(str.c_str(), prefix.c_str(), prefixLength) == 0;
+}
+
bool EndsWith(const std::string &str, const char *suffix)
{
const auto len = strlen(suffix);
diff --git a/src/common/string_utils.h b/src/common/string_utils.h
index 4437fb3..07bf1ff 100644
--- a/src/common/string_utils.h
+++ b/src/common/string_utils.h
@@ -49,6 +49,10 @@
Optional<std::vector<wchar_t>> WidenString(size_t length, const char *cString);
// Check if the string str begins with the given prefix.
+// The comparison is case sensitive.
+bool BeginsWith(const std::string &str, const std::string &prefix);
+
+// Check if the string str begins with the given prefix.
// Prefix may not be NULL and needs to be NULL terminated.
// The comparison is case sensitive.
bool BeginsWith(const std::string &str, const char *prefix);
@@ -58,6 +62,11 @@
// The comparison is case sensitive.
bool BeginsWith(const char *str, const char *prefix);
+// Check if the string str begins with the first prefixLength characters of the given prefix.
+// The length of the prefix string should be greater than or equal to prefixLength.
+// The comparison is case sensitive.
+bool BeginsWith(const std::string &str, const std::string &prefix, const size_t prefixLength);
+
// Check if the string str ends with the given suffix.
// Suffix may not be NUL and needs to be NULL terminated.
// The comparison is case sensitive.
diff --git a/src/common/string_utils_unittest.cpp b/src/common/string_utils_unittest.cpp
index e7ce8a6..04e4b1f 100644
--- a/src/common/string_utils_unittest.cpp
+++ b/src/common/string_utils_unittest.cpp
@@ -138,18 +138,73 @@
// Note: ReadFileToString is harder to test
-
-TEST(StringUtilsTest, BeginsEndsWith)
+class BeginsWithTest : public testing::Test
{
- ASSERT_FALSE(BeginsWith("foo", "bar"));
- ASSERT_FALSE(BeginsWith("", "foo"));
- ASSERT_FALSE(BeginsWith("foo", "foobar"));
+ public:
+ BeginsWithTest() : mMode(TestMode::CHAR_ARRAY) {}
- ASSERT_TRUE(BeginsWith("foobar", "foo"));
- ASSERT_TRUE(BeginsWith("foobar", ""));
- ASSERT_TRUE(BeginsWith("foo", "foo"));
- ASSERT_TRUE(BeginsWith("", ""));
+ enum class TestMode
+ {
+ CHAR_ARRAY,
+ STRING_AND_CHAR_ARRAY,
+ STRING
+ };
+ void setMode(TestMode mode) { mMode = mode; }
+
+ bool runBeginsWith(const char *str, const char *prefix)
+ {
+ if (mMode == TestMode::CHAR_ARRAY)
+ {
+ return BeginsWith(str, prefix);
+ }
+ if (mMode == TestMode::STRING_AND_CHAR_ARRAY)
+ {
+ return BeginsWith(std::string(str), prefix);
+ }
+ return BeginsWith(std::string(str), std::string(prefix));
+ }
+
+ void runTest()
+ {
+ ASSERT_FALSE(runBeginsWith("foo", "bar"));
+ ASSERT_FALSE(runBeginsWith("", "foo"));
+ ASSERT_FALSE(runBeginsWith("foo", "foobar"));
+
+ ASSERT_TRUE(runBeginsWith("foobar", "foo"));
+ ASSERT_TRUE(runBeginsWith("foobar", ""));
+ ASSERT_TRUE(runBeginsWith("foo", "foo"));
+ ASSERT_TRUE(runBeginsWith("", ""));
+ }
+
+ private:
+ TestMode mMode;
+};
+
+// Test that BeginsWith works correctly for const char * arguments.
+TEST_F(BeginsWithTest, CharArrays)
+{
+ setMode(TestMode::CHAR_ARRAY);
+ runTest();
+}
+
+// Test that BeginsWith works correctly for std::string and const char * arguments.
+TEST_F(BeginsWithTest, StringAndCharArray)
+{
+ setMode(TestMode::STRING_AND_CHAR_ARRAY);
+ runTest();
+}
+
+// Test that BeginsWith works correctly for std::string arguments.
+TEST_F(BeginsWithTest, Strings)
+{
+ setMode(TestMode::STRING);
+ runTest();
+}
+
+// Test that EndsWith works correctly.
+TEST(EndsWithTest, EndsWith)
+{
ASSERT_FALSE(EndsWith("foo", "bar"));
ASSERT_FALSE(EndsWith("", "bar"));
ASSERT_FALSE(EndsWith("foo", "foobar"));
@@ -160,4 +215,4 @@
ASSERT_TRUE(EndsWith("", ""));
}
-}
\ No newline at end of file
+} // anonymous namespace
diff --git a/src/common/utilities.cpp b/src/common/utilities.cpp
index efb5323..677e800 100644
--- a/src/common/utilities.cpp
+++ b/src/common/utilities.cpp
@@ -770,19 +770,33 @@
return name.substr(0, baseNameLength);
}
-unsigned int ParseAndStripArrayIndex(std::string *name)
+unsigned int ParseArrayIndex(const std::string &name, size_t *nameLengthWithoutArrayIndexOut)
{
+ ASSERT(nameLengthWithoutArrayIndexOut != nullptr);
unsigned int subscript = GL_INVALID_INDEX;
// Strip any trailing array operator and retrieve the subscript
- size_t open = name->find_last_of('[');
- size_t close = name->find_last_of(']');
- if (open != std::string::npos && close == name->length() - 1)
+ size_t open = name.find_last_of('[');
+ if (open != std::string::npos && name.back() == ']')
{
- subscript = atoi(name->c_str() + open + 1);
- name->erase(open);
+ bool indexIsValidDecimalNumber = true;
+ for (size_t i = open + 1; i < name.length() - 1u; ++i)
+ {
+ if (!isdigit(name[i]))
+ {
+ indexIsValidDecimalNumber = false;
+ break;
+ }
+ }
+ if (indexIsValidDecimalNumber)
+ {
+ subscript = atoi(name.c_str() + open + 1);
+ *nameLengthWithoutArrayIndexOut = open;
+ return subscript;
+ }
}
+ *nameLengthWithoutArrayIndexOut = name.length();
return subscript;
}
diff --git a/src/common/utilities.h b/src/common/utilities.h
index e26f781..db0624f 100644
--- a/src/common/utilities.h
+++ b/src/common/utilities.h
@@ -70,7 +70,10 @@
bool IsTriangleMode(GLenum drawMode);
bool IsIntegerFormat(GLenum unsizedFormat);
-unsigned int ParseAndStripArrayIndex(std::string *name);
+// Return the array index at the end of name, and write the length of name before the final array
+// index into nameLengthWithoutArrayIndexOut. In case name doesn't include an array index, return
+// GL_INVALID_INDEX and write the length of the original string.
+unsigned int ParseArrayIndex(const std::string &name, size_t *nameLengthWithoutArrayIndexOut);
struct UniformTypeInfo final : angle::NonCopyable
{
diff --git a/src/common/utilities_unittest.cpp b/src/common/utilities_unittest.cpp
index 7447ae4..571c578 100644
--- a/src/common/utilities_unittest.cpp
+++ b/src/common/utilities_unittest.cpp
@@ -75,4 +75,97 @@
EXPECT_TRUE(indices.empty());
}
+// Parse a string without any index.
+TEST(ParseArrayIndex, NoArrayIndex)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(3u, nameLengthWithoutArrayIndex);
}
+
+// Parse an empty string for an array index.
+TEST(ParseArrayIndex, EmptyString)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(0u, nameLengthWithoutArrayIndex);
+}
+
+// A valid array index is parsed correctly from the end of the string.
+TEST(ParseArrayIndex, ArrayIndex)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(123u, gl::ParseArrayIndex("foo[123]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(3u, nameLengthWithoutArrayIndex);
+}
+
+// An array index from the middle of the string is not parsed.
+TEST(ParseArrayIndex, ArrayIndexInMiddle)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[123].bar", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(12u, nameLengthWithoutArrayIndex);
+}
+
+// Trailing whitespace in the parsed string is taken into account.
+TEST(ParseArrayIndex, TrailingWhitespace)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[123] ", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(9u, nameLengthWithoutArrayIndex);
+}
+
+// Only the last index is parsed.
+TEST(ParseArrayIndex, MultipleArrayIndices)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(34u, gl::ParseArrayIndex("foo[12][34]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(7u, nameLengthWithoutArrayIndex);
+}
+
+// GetProgramResourceLocation spec in GLES 3.1 November 2016 page 87 mentions "decimal" integer.
+// So an integer in hexadecimal format should not parse as an array index.
+TEST(ParseArrayIndex, HexArrayIndex)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[0xff]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(9u, nameLengthWithoutArrayIndex);
+}
+
+// GetProgramResourceLocation spec in GLES 3.1 November 2016 page 87 mentions that the array
+// index should not contain a leading plus sign.
+TEST(ParseArrayIndex, ArrayIndexLeadingPlus)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[+1]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(7u, nameLengthWithoutArrayIndex);
+}
+
+// GetProgramResourceLocation spec in GLES 3.1 November 2016 page 87 says that index should not
+// contain whitespace. Test leading whitespace.
+TEST(ParseArrayIndex, ArrayIndexLeadingWhiteSpace)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[ 0]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(7u, nameLengthWithoutArrayIndex);
+}
+
+// GetProgramResourceLocation spec in GLES 3.1 November 2016 page 87 says that index should not
+// contain whitespace. Test trailing whitespace.
+TEST(ParseArrayIndex, ArrayIndexTrailingWhiteSpace)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[0 ]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(7u, nameLengthWithoutArrayIndex);
+}
+
+// GetProgramResourceLocation spec in GLES 3.1 November 2016 page 87 says that index should only
+// contain an integer.
+TEST(ParseArrayIndex, ArrayIndexBogus)
+{
+ size_t nameLengthWithoutArrayIndex;
+ EXPECT_EQ(GL_INVALID_INDEX, gl::ParseArrayIndex("foo[0bogus]", &nameLengthWithoutArrayIndex));
+ EXPECT_EQ(11u, nameLengthWithoutArrayIndex);
+}
+
+} // anonymous namespace