Skip to content

Commit b01f4a5

Browse files
authored
Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter#173771)
This PR improves the OpenGL error handling and procedure table interface in the Impeller renderer by changing `const char*` to `std::string_view` for better memory safety. ### Testing: Added unit tests to cover `GLErrorToString` return values and `string_view` behavior ### Issues Fixed flutter#135922 This PR addresses technical debt by improving string handling in the OpenGL backend, and type safety without breaking existing functionality. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
1 parent df8e02d commit b01f4a5

File tree

4 files changed

+53
-10
lines changed

4 files changed

+53
-10
lines changed

engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
namespace impeller {
1717

18-
const char* GLErrorToString(GLenum value) {
18+
std::string_view GLErrorToString(GLenum value) {
1919
switch (value) {
2020
case GL_NO_ERROR:
2121
return "GL_NO_ERROR";
@@ -89,7 +89,7 @@ ProcTableGLES::ProcTableGLES( // NOLINT(google-readability-function-size)
8989
}
9090

9191
#define IMPELLER_PROC(proc_ivar) \
92-
if (auto fn_ptr = resolver(proc_ivar.name)) { \
92+
if (auto fn_ptr = resolver(proc_ivar.name.data())) { \
9393
proc_ivar.function = \
9494
reinterpret_cast<decltype(proc_ivar.function)>(fn_ptr); \
9595
proc_ivar.error_fn = error_fn; \
@@ -115,7 +115,7 @@ ProcTableGLES::ProcTableGLES( // NOLINT(google-readability-function-size)
115115
#undef IMPELLER_PROC
116116

117117
#define IMPELLER_PROC(proc_ivar) \
118-
if (auto fn_ptr = resolver(proc_ivar.name)) { \
118+
if (auto fn_ptr = resolver(proc_ivar.name.data())) { \
119119
proc_ivar.function = \
120120
reinterpret_cast<decltype(proc_ivar.function)>(fn_ptr); \
121121
proc_ivar.error_fn = error_fn; \

engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include <functional>
99
#include <string>
10+
#include <string_view>
1011

1112
#include "flutter/fml/logging.h"
1213
#include "flutter/fml/mapping.h"
@@ -19,16 +20,17 @@
1920

2021
namespace impeller {
2122

22-
const char* GLErrorToString(GLenum value);
23+
std::string_view GLErrorToString(GLenum value);
2324
bool GLErrorIsFatal(GLenum value);
2425

2526
struct AutoErrorCheck {
2627
const PFNGLGETERRORPROC error_fn;
2728

28-
// TODO(135922) Change to string_view.
29-
const char* name;
29+
/// Name of the GL call being wrapped.
30+
/// should not be stored beyond the caller's lifetime.
31+
std::string_view name;
3032

31-
AutoErrorCheck(PFNGLGETERRORPROC error, const char* name)
33+
AutoErrorCheck(PFNGLGETERRORPROC error, std::string_view name)
3234
: error_fn(error), name(name) {}
3335

3436
~AutoErrorCheck() {
@@ -80,8 +82,7 @@ struct GLProc {
8082
//----------------------------------------------------------------------------
8183
/// The name of the GL function.
8284
///
83-
// TODO(135922) Change to string_view.
84-
const char* name = nullptr;
85+
std::string_view name = {};
8586

8687
//----------------------------------------------------------------------------
8788
/// The pointer to the GL function.

engine/src/flutter/impeller/renderer/backend/gles/test/proc_table_gles_unittests.cc

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,5 +33,47 @@ TEST(ProcTableGLES, ResolvesCorrectClearDepthProcOnDesktopGL) {
3333
FOR_EACH_IMPELLER_ES_ONLY_PROC(EXPECT_UNAVAILABLE);
3434
}
3535

36+
TEST(GLErrorToString, ReturnsCorrectStringForKnownErrors) {
37+
EXPECT_EQ(GLErrorToString(GL_NO_ERROR), "GL_NO_ERROR");
38+
EXPECT_EQ(GLErrorToString(GL_INVALID_ENUM), "GL_INVALID_ENUM");
39+
EXPECT_EQ(GLErrorToString(GL_INVALID_VALUE), "GL_INVALID_VALUE");
40+
EXPECT_EQ(GLErrorToString(GL_INVALID_OPERATION), "GL_INVALID_OPERATION");
41+
EXPECT_EQ(GLErrorToString(GL_INVALID_FRAMEBUFFER_OPERATION),
42+
"GL_INVALID_FRAMEBUFFER_OPERATION");
43+
EXPECT_EQ(GLErrorToString(GL_FRAMEBUFFER_COMPLETE),
44+
"GL_FRAMEBUFFER_COMPLETE");
45+
EXPECT_EQ(GLErrorToString(GL_OUT_OF_MEMORY), "GL_OUT_OF_MEMORY");
46+
}
47+
48+
TEST(GLErrorToString, ReturnsUnknownForInvalidError) {
49+
// Test with an invalid error code
50+
GLenum invalid_error = 0x9999;
51+
EXPECT_EQ(GLErrorToString(invalid_error), "Unknown.");
52+
}
53+
54+
TEST(GLErrorToString, ReturnValueIsValidStringView) {
55+
// Test that the returned string_view is valid and non-empty
56+
auto result = GLErrorToString(GL_NO_ERROR);
57+
EXPECT_FALSE(result.empty());
58+
EXPECT_NE(result.data(), nullptr);
59+
60+
// Test that we can compare with string literals
61+
EXPECT_TRUE(result == "GL_NO_ERROR");
62+
}
63+
64+
TEST(GLProc, NameFieldWorksWithStringView) {
65+
GLProc<void()> proc;
66+
67+
// Test setting name with string literal
68+
const char* literal = "glTestFunction";
69+
proc.name = literal;
70+
71+
EXPECT_EQ(proc.name, "glTestFunction");
72+
EXPECT_FALSE(proc.name.empty());
73+
74+
// Test that the string_view properly references the original data
75+
EXPECT_EQ(proc.name.data(), literal);
76+
}
77+
3678
} // namespace testing
3779
} // namespace impeller

engine/src/flutter/impeller/toolkit/glvk/proc_table.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ ProcTable::ProcTable(const Resolver& resolver) {
2020
}
2121

2222
#define GLVK_PROC(proc_ivar) \
23-
if (auto fn_ptr = resolver(proc_ivar.name)) { \
23+
if (auto fn_ptr = resolver(proc_ivar.name.data())) { \
2424
proc_ivar.function = \
2525
reinterpret_cast<decltype(proc_ivar.function)>(fn_ptr); \
2626
proc_ivar.error_fn = error_fn; \

0 commit comments

Comments
 (0)