From d1774ef4d2f747230b8bae088506e68dade502a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 1 Mar 2021 17:37:14 +0100 Subject: [PATCH] Platform: replace UGLY error-prone C string handling with StringViews. We no longer have to use sizeof("...") to avoid useless strlen calls or check for nulls because the StringView APIs are ACTUALLY SANE and not a full of nasty surprises and performance / security pitfalls like with both the C and C++ standard library functions. Good riddance. --- src/Magnum/Platform/GlfwApplication.cpp | 27 ++++++------------- src/Magnum/Platform/Sdl2Application.cpp | 22 +++++---------- .../Platform/WindowlessEglApplication.cpp | 24 ++++++++--------- .../Platform/WindowlessGlxApplication.cpp | 14 +++------- .../Platform/WindowlessWglApplication.cpp | 17 ++++-------- 5 files changed, 35 insertions(+), 69 deletions(-) diff --git a/src/Magnum/Platform/GlfwApplication.cpp b/src/Magnum/Platform/GlfwApplication.cpp index 5e51f14ac..1527651d2 100644 --- a/src/Magnum/Platform/GlfwApplication.cpp +++ b/src/Magnum/Platform/GlfwApplication.cpp @@ -28,7 +28,6 @@ #include "GlfwApplication.h" -#include #include #include #include @@ -498,9 +497,7 @@ bool GlfwApplication::tryCreate(const Configuration& configuration, const GLConf succeeds, make the context current so we can query GL_VENDOR below. If we are on Wayland, this is causing a segfault; a blinking window is acceptable in this case. */ - constexpr const char waylandString[] = "wayland"; - const char* const xdgSessionType = std::getenv("XDG_SESSION_TYPE"); - if(!xdgSessionType || std::strncmp(xdgSessionType, waylandString, sizeof(waylandString)) != 0) + if(std::getenv("XDG_SESSION_TYPE") != "wayland"_s) glfwWindowHint(GLFW_VISIBLE, false); else if(_verboseLog) Warning{} << "Platform::GlfwApplication: Wayland detected, GL context has to be created with the window visible and may cause flicker on startup"; @@ -515,27 +512,19 @@ bool GlfwApplication::tryCreate(const Configuration& configuration, const GLConf version, they force the version to the one specified, which is completely useless behavior. */ #ifndef CORRADE_TARGET_APPLE - constexpr static const char nvidiaVendorString[] = "NVIDIA Corporation"; - #ifdef CORRADE_TARGET_WINDOWS - constexpr static const char intelVendorString[] = "Intel"; - #endif - constexpr static const char amdVendorString[] = "ATI Technologies Inc."; - const char* vendorString; + Containers::StringView vendorString; #endif if(glConfiguration.version() == GL::Version::None && (!_window #ifndef CORRADE_TARGET_APPLE - /* If context creation fails *really bad*, glGetString() may actually - return nullptr. Check for that to avoid crashes deep inside - strncmp(). Sorry about the UGLY code, HOPEFULLY THERE WON'T BE MORE - WORKAROUNDS */ + /* Sorry about the UGLY code, HOPEFULLY THERE WON'T BE MORE WORKAROUNDS */ || (vendorString = reinterpret_cast(glGetString(GL_VENDOR)), - vendorString && (std::strncmp(vendorString, nvidiaVendorString, sizeof(nvidiaVendorString)) == 0 || + (vendorString == "NVIDIA Corporation"_s || #ifdef CORRADE_TARGET_WINDOWS - std::strncmp(vendorString, intelVendorString, sizeof(intelVendorString)) == 0 || - #endif - std::strncmp(vendorString, amdVendorString, sizeof(amdVendorString)) == 0) - && !_context->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s)) + vendorString == "Intel"_s || #endif + vendorString == "ATI Technologies Inc."_s) + && !_context->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s)) + #endif )) { /* Don't print any warning when doing the workaround, because the bug will be there probably forever */ diff --git a/src/Magnum/Platform/Sdl2Application.cpp b/src/Magnum/Platform/Sdl2Application.cpp index 3b815f653..1552d9afc 100644 --- a/src/Magnum/Platform/Sdl2Application.cpp +++ b/src/Magnum/Platform/Sdl2Application.cpp @@ -26,7 +26,7 @@ #include "Sdl2Application.h" -#include +#include /** @todo remove, needed for std::strlen() in TextInputEvent */ #ifdef CORRADE_TARGET_CLANG_CL /* SDL does #pragma pack(push,8) and #pragma pack(pop,8) in different headers (begin_code.h and end_code.h) and clang-cl doesn't like that, even though it @@ -545,26 +545,18 @@ bool Sdl2Application::tryCreate(const Configuration& configuration, const GLConf version, they force the version to the one specified, which is completely useless behavior. */ #ifndef CORRADE_TARGET_APPLE - constexpr static const char nvidiaVendorString[] = "NVIDIA Corporation"; - #ifdef CORRADE_TARGET_WINDOWS - constexpr static const char intelVendorString[] = "Intel"; - #endif - constexpr static const char amdVendorString[] = "ATI Technologies Inc."; - const char* vendorString; + Containers::StringView vendorString; #endif if(glConfiguration.version() == GL::Version::None && (!_glContext #ifndef CORRADE_TARGET_APPLE - /* If context creation fails *really bad*, glGetString() may actually - return nullptr. Check for that to avoid crashes deep inside - strncmp(). Sorry about the UGLY code, HOPEFULLY THERE WON'T BE MORE - WORKAROUNDS */ + /* Sorry about the UGLY code, HOPEFULLY THERE WON'T BE MORE WORKAROUNDS */ || (vendorString = reinterpret_cast(glGetString(GL_VENDOR)), - vendorString && (std::strncmp(vendorString, nvidiaVendorString, sizeof(nvidiaVendorString)) == 0 || + (vendorString == "NVIDIA Corporation"_s || #ifdef CORRADE_TARGET_WINDOWS - std::strncmp(vendorString, intelVendorString, sizeof(intelVendorString)) == 0 || + vendorString == "Intel"_s || #endif - std::strncmp(vendorString, amdVendorString, sizeof(amdVendorString)) == 0) - && !_context->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s)) + vendorString == "ATI Technologies Inc."_s) + && !_context->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s)) #endif )) { /* Don't print any warning when doing the workaround, because the bug diff --git a/src/Magnum/Platform/WindowlessEglApplication.cpp b/src/Magnum/Platform/WindowlessEglApplication.cpp index 36adfadb0..dbcf71e64 100644 --- a/src/Magnum/Platform/WindowlessEglApplication.cpp +++ b/src/Magnum/Platform/WindowlessEglApplication.cpp @@ -27,7 +27,7 @@ #include "WindowlessEglApplication.h" -#include +#include /** @todo used by extensionSupported(), cleann up */ #include #include #include @@ -346,13 +346,13 @@ WindowlessEglContext::WindowlessEglContext(const Configuration& configuration, G #endif #if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_WEBGL) - const char* version = eglQueryString(_display, EGL_VERSION); + const Containers::StringView version = eglQueryString(_display, EGL_VERSION); /* SwiftShader 3.3.0.1 blows up on encountering EGL_CONTEXT_FLAGS_KHR with a zero value, so erase these. It also doesn't handle them as correct flags, but instead checks for the whole value, so a combination won't work either: https://github.com/google/swiftshader/blob/5fb5e817a20d3e60f29f7338493f922b5ac9d7c4/src/OpenGL/libEGL/libEGL.cpp#L794-L8104 */ - if(!(UnsignedLong(flags) & 0xffffffffu) && version && std::strstr(version, "SwiftShader") != nullptr && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("swiftshader-no-empty-egl-context-flags"_s))) { + if(!(UnsignedLong(flags) & 0xffffffffu) && version.contains("SwiftShader"_s) && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("swiftshader-no-empty-egl-context-flags"_s))) { auto& contextFlags = attributes[Containers::arraySize(attributes) - 3]; CORRADE_INTERNAL_ASSERT(contextFlags == EGL_CONTEXT_FLAGS_KHR); contextFlags = EGL_NONE; @@ -401,15 +401,13 @@ WindowlessEglContext::WindowlessEglContext(const Configuration& configuration, G /* The workaround check is the last so it doesn't appear in workaround list on unrelated drivers */ - constexpr static const char nvidiaVendorString[] = "NVIDIA Corporation"; - constexpr static const char amdVendorString[] = "ATI Technologies Inc."; - const char* const vendorString = reinterpret_cast(glGetString(GL_VENDOR)); - /* If context creation fails *really bad*, glGetString() may actually - return nullptr. Check for that to avoid crashes deep inside - strncmp() */ - if(vendorString && (std::strncmp(vendorString, nvidiaVendorString, sizeof(nvidiaVendorString)) == 0 || - std::strncmp(vendorString, amdVendorString, sizeof(amdVendorString)) == 0) && - (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) + const Containers::StringView vendorString = reinterpret_cast(glGetString(GL_VENDOR)); + /* Unlike GLFW/SDL2/WGL there's no Intel here because EGL doesn't work + with native drivers on Windows, only though ANGLE etc, and those + don't suffer from this issue. */ + if((vendorString == "NVIDIA Corporation"_s || + vendorString == "ATI Technologies Inc."_s) + && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) { /* Destroy the core context and create a compatibility one */ eglDestroyContext(_display, _context); @@ -442,7 +440,7 @@ WindowlessEglContext::WindowlessEglContext(const Configuration& configuration, G #if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_WEBGL) /* SwiftShader 3.3.0.1 needs some pbuffer, otherwise it crashes somewhere deep inside when making the context current */ - if(version && std::strstr(version, "SwiftShader") != nullptr && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("swiftshader-egl-context-needs-pbuffer"_s))) { + if(version.contains("SwiftShader"_s) && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("swiftshader-egl-context-needs-pbuffer"_s))) { EGLint surfaceAttributes[] = { EGL_WIDTH, 32, EGL_HEIGHT, 32, diff --git a/src/Magnum/Platform/WindowlessGlxApplication.cpp b/src/Magnum/Platform/WindowlessGlxApplication.cpp index 01bea105a..71e3e07db 100644 --- a/src/Magnum/Platform/WindowlessGlxApplication.cpp +++ b/src/Magnum/Platform/WindowlessGlxApplication.cpp @@ -26,7 +26,6 @@ #include "WindowlessGlxApplication.h" -#include #include #include #include @@ -186,15 +185,10 @@ WindowlessGlxContext::WindowlessGlxContext(const WindowlessGlxContext::Configura /* The workaround check is the last so it doesn't appear in workaround list on unrelated drivers */ - constexpr static const char nvidiaVendorString[] = "NVIDIA Corporation"; - constexpr static const char amdVendorString[] = "ATI Technologies Inc."; - const char* const vendorString = reinterpret_cast(glGetString(GL_VENDOR)); - /* If context creation fails *really bad*, glGetString() may actually - return nullptr. Check for that to avoid crashes deep inside - strncmp() */ - if(vendorString && (std::strncmp(vendorString, nvidiaVendorString, sizeof(nvidiaVendorString)) == 0 || - std::strncmp(vendorString, amdVendorString, sizeof(amdVendorString)) == 0) && - (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) + const Containers::StringView vendorString = reinterpret_cast(glGetString(GL_VENDOR)); + if((vendorString == "NVIDIA Corporation"_s || + vendorString == "ATI Technologies Inc."_s) + && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) { /* Destroy the core context and create a compatibility one */ glXDestroyContext(_display, _context); diff --git a/src/Magnum/Platform/WindowlessWglApplication.cpp b/src/Magnum/Platform/WindowlessWglApplication.cpp index 493e94b26..13ad08dc5 100644 --- a/src/Magnum/Platform/WindowlessWglApplication.cpp +++ b/src/Magnum/Platform/WindowlessWglApplication.cpp @@ -25,7 +25,6 @@ #include "WindowlessWglApplication.h" -#include #include #include #include @@ -183,17 +182,11 @@ WindowlessWglContext::WindowlessWglContext(const Configuration& configuration, G /* The workaround check is the last so it doesn't appear in workaround list on unrelated drivers */ - constexpr static const char nvidiaVendorString[] = "NVIDIA Corporation"; - constexpr static const char intelVendorString[] = "Intel"; - constexpr static const char amdVendorString[] = "ATI Technologies Inc."; - const char* const vendorString = reinterpret_cast(glGetString(GL_VENDOR)); - /* If context creation fails *really bad*, glGetString() may actually - return nullptr. Check for that to avoid crashes deep inside - strncmp() */ - if(vendorString && (std::strncmp(vendorString, nvidiaVendorString, sizeof(nvidiaVendorString)) == 0 || - std::strncmp(vendorString, intelVendorString, sizeof(intelVendorString)) == 0 || - std::strncmp(vendorString, amdVendorString, sizeof(amdVendorString)) == 0) && - (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) + const Containers::StringView vendorString = reinterpret_cast(glGetString(GL_VENDOR)); + if((vendorString == "NVIDIA Corporation"_s || + vendorString == "Intel"_s || + vendorString == "ATI Technologies Inc."_s) + && (!magnumContext || !magnumContext->isDriverWorkaroundDisabled("no-forward-compatible-core-context"_s))) { /* Destroy the core context and create a compatibility one */ wglDeleteContext(_context);