From 4aee858fef7a209a41c653f03e5be822f8546a49 Mon Sep 17 00:00:00 2001 From: Chris Thrasher Date: Wed, 10 Jul 2024 14:01:59 -0600 Subject: [PATCH] Add new thread-safe error logging API --- include/SFML/System/Err.hpp | 6 +++ src/SFML/System/CMakeLists.txt | 1 + src/SFML/System/Err.cpp | 29 +++++++++++++ src/SFML/System/Logging.hpp | 62 ++++++++++++++++++++++++++++ src/SFML/Window/Context.cpp | 8 ++-- src/SFML/Window/Cursor.cpp | 4 +- src/SFML/Window/EGLCheck.cpp | 16 +++++-- src/SFML/Window/EglContext.cpp | 14 +++---- src/SFML/Window/GlContext.cpp | 62 +++++++++++++++++++--------- src/SFML/Window/SensorManager.cpp | 7 ++-- src/SFML/Window/Window.cpp | 6 +-- src/SFML/Window/WindowImpl.cpp | 15 ++++--- test/CMakeLists.txt | 2 +- test/{System => Window}/Err.test.cpp | 27 ++++++++++++ 14 files changed, 210 insertions(+), 49 deletions(-) create mode 100644 src/SFML/System/Logging.hpp rename test/{System => Window}/Err.test.cpp (59%) diff --git a/include/SFML/System/Err.hpp b/include/SFML/System/Err.hpp index d330cc2a5..37e69b0f8 100644 --- a/include/SFML/System/Err.hpp +++ b/include/SFML/System/Err.hpp @@ -40,6 +40,12 @@ namespace sf //////////////////////////////////////////////////////////// [[nodiscard]] SFML_SYSTEM_API std::ostream& err(); +//////////////////////////////////////////////////////////// +/// \brief Specify buffer to which warnings and errors are logged +/// +//////////////////////////////////////////////////////////// +SFML_SYSTEM_API void setErrorBuffer(std::streambuf* streamBuffer); + } // namespace sf diff --git a/src/SFML/System/CMakeLists.txt b/src/SFML/System/CMakeLists.txt index 616141924..66fcbb5ae 100644 --- a/src/SFML/System/CMakeLists.txt +++ b/src/SFML/System/CMakeLists.txt @@ -12,6 +12,7 @@ set(SRC ${INCROOT}/Err.hpp ${INCROOT}/Export.hpp ${INCROOT}/InputStream.hpp + ${SRCROOT}/Logging.hpp ${INCROOT}/NativeActivity.hpp ${SRCROOT}/Sleep.cpp ${INCROOT}/Sleep.hpp diff --git a/src/SFML/System/Err.cpp b/src/SFML/System/Err.cpp index a210f91fc..ffa1318aa 100644 --- a/src/SFML/System/Err.cpp +++ b/src/SFML/System/Err.cpp @@ -26,8 +26,10 @@ // Headers //////////////////////////////////////////////////////////// #include +#include #include +#include #include #include @@ -96,6 +98,25 @@ private: namespace sf { +namespace priv +{ +//////////////////////////////////////////////////////////// +std::mutex& errorStreamMutex() +{ + static std::mutex mutex; + return mutex; +} + + +//////////////////////////////////////////////////////////// +std::ostream& errorStream() +{ + static std::ostream errorStream(std::cerr.rdbuf()); + return errorStream; +} +} // namespace priv + + //////////////////////////////////////////////////////////// std::ostream& err() { @@ -106,4 +127,12 @@ std::ostream& err() } +//////////////////////////////////////////////////////////// +void setErrorBuffer(std::streambuf* streamBuffer) +{ + const std::lock_guard lock(priv::errorStreamMutex()); + priv::errorStream().rdbuf(streamBuffer); +} + + } // namespace sf diff --git a/src/SFML/System/Logging.hpp b/src/SFML/System/Logging.hpp new file mode 100644 index 000000000..2fee5533b --- /dev/null +++ b/src/SFML/System/Logging.hpp @@ -0,0 +1,62 @@ +//////////////////////////////////////////////////////////// +// +// SFML - Simple and Fast Multimedia Library +// Copyright (C) 2007-2024 Laurent Gomila (laurent@sfml-dev.org) +// +// This software is provided 'as-is', without any express or implied warranty. +// In no event will the authors be held liable for any damages arising from the use of this software. +// +// Permission is granted to anyone to use this software for any purpose, +// including commercial applications, and to alter it and redistribute it freely, +// subject to the following restrictions: +// +// 1. The origin of this software must not be misrepresented; +// you must not claim that you wrote the original software. +// If you use this software in a product, an acknowledgment +// in the product documentation would be appreciated but is not required. +// +// 2. Altered source versions must be plainly marked as such, +// and must not be misrepresented as being the original software. +// +// 3. This notice may not be removed or altered from any source distribution. +// +//////////////////////////////////////////////////////////// + +#pragma once + +//////////////////////////////////////////////////////////// +// Headers +//////////////////////////////////////////////////////////// +#include + +#include +#include + + +namespace sf::priv +{ +//////////////////////////////////////////////////////////// +/// \brief Get mutex for locking error stream +/// +//////////////////////////////////////////////////////////// +SFML_SYSTEM_API std::mutex& errorStreamMutex(); + +//////////////////////////////////////////////////////////// +/// \brief Get error stream +/// +//////////////////////////////////////////////////////////// +SFML_SYSTEM_API std::ostream& errorStream(); + +//////////////////////////////////////////////////////////// +/// \brief Log warning or error to a given stream +/// +//////////////////////////////////////////////////////////// +template +void log(Args&&... args) +{ + const std::lock_guard lock(errorStreamMutex()); + (errorStream() << ... << std::forward(args)); + errorStream() << std::endl; +} + +} // namespace sf::priv diff --git a/src/SFML/Window/Context.cpp b/src/SFML/Window/Context.cpp index 2af3d5322..4c1a50037 100644 --- a/src/SFML/Window/Context.cpp +++ b/src/SFML/Window/Context.cpp @@ -28,7 +28,7 @@ #include #include -#include +#include #include #include @@ -50,7 +50,7 @@ namespace sf Context::Context() : m_context(priv::GlContext::create()) { if (!setActive(true)) - err() << "Failed to set context as active during construction" << std::endl; + priv::log("Failed to set context as active during construction"); } @@ -58,7 +58,7 @@ Context::Context() : m_context(priv::GlContext::create()) Context::~Context() { if (m_context && !setActive(false)) - err() << "Failed to set context as inactive during destruction" << std::endl; + priv::log("Failed to set context as inactive during destruction"); } @@ -144,7 +144,7 @@ Context::Context(const ContextSettings& settings, const Vector2u& size) : m_context(priv::GlContext::create(settings, size)) { if (!setActive(true)) - err() << "Failed to set context as active during construction" << std::endl; + priv::log("Failed to set context as active during construction"); } } // namespace sf diff --git a/src/SFML/Window/Cursor.cpp b/src/SFML/Window/Cursor.cpp index e3769fa6b..b5f8ad0a6 100644 --- a/src/SFML/Window/Cursor.cpp +++ b/src/SFML/Window/Cursor.cpp @@ -28,7 +28,7 @@ #include #include -#include +#include #include #include @@ -60,7 +60,7 @@ std::optional Cursor::loadFromPixels(const std::uint8_t* pixels, Vector2 { if ((pixels == nullptr) || (size.x == 0) || (size.y == 0)) { - err() << "Failed to load cursor from pixels (invalid arguments)" << std::endl; + priv::log("Failed to load cursor from pixels (invalid arguments)"); return std::nullopt; } diff --git a/src/SFML/Window/EGLCheck.cpp b/src/SFML/Window/EGLCheck.cpp index 8f050d8d0..5c9a90d99 100644 --- a/src/SFML/Window/EGLCheck.cpp +++ b/src/SFML/Window/EGLCheck.cpp @@ -28,7 +28,7 @@ //////////////////////////////////////////////////////////// #include -#include +#include #include @@ -163,9 +163,17 @@ void eglCheckError(const std::filesystem::path& file, unsigned int line, std::st } // Log the error - err() << "An internal EGL call failed in " << file.filename() << " (" << line << ") : " - << "\nExpression:\n " << expression << "\nError description:\n " << error << "\n " << description << '\n' - << std::endl; + priv::log("An internal EGL call failed in ", + file.filename(), + " (", + line, + ") : ", + "\nExpression:\n ", + expression, + "\nError description:\n ", + error, + "\n ", + description); } } diff --git a/src/SFML/Window/EglContext.cpp b/src/SFML/Window/EglContext.cpp index f42a41b14..a84f701e6 100644 --- a/src/SFML/Window/EglContext.cpp +++ b/src/SFML/Window/EglContext.cpp @@ -29,7 +29,7 @@ #include #include -#include +#include #include #include @@ -93,7 +93,7 @@ void ensureInit() { // At this point, the failure is unrecoverable // Dump a message to the console and let the application terminate - sf::err() << "Failed to load EGL entry points" << std::endl; + sf::priv::log("Failed to load EGL entry points"); assert(false); @@ -177,9 +177,9 @@ EglContext::EglContext(EglContext* /*shared*/, const ContextSettings& /*settings { EglContextImpl::ensureInit(); - sf::err() << "Warning: context has not been initialized. The constructor EglContext(shared, settings, size) is " - "currently not implemented." - << std::endl; + sf::priv::log( + "Warning: context has not been initialized. The constructor EglContext(shared, settings, size) is " + "currently not implemented."); } @@ -424,7 +424,7 @@ XVisualInfo EglContext::selectBestVisual(::Display* xDisplay, unsigned int bitsP if (nativeVisualId == 0) { // Should never happen... - err() << "No EGL visual found. You should check your graphics driver" << std::endl; + priv::log("No EGL visual found. You should check your graphics driver"); return {}; } @@ -439,7 +439,7 @@ XVisualInfo EglContext::selectBestVisual(::Display* xDisplay, unsigned int bitsP if (visualCount == 0) { // Can't happen... - err() << "No X11 visual found. Bug in your EGL implementation ?" << std::endl; + priv::log("No X11 visual found. Bug in your EGL implementation ?"); return {}; } diff --git a/src/SFML/Window/GlContext.cpp b/src/SFML/Window/GlContext.cpp index bd01335fa..69b9e1b72 100644 --- a/src/SFML/Window/GlContext.cpp +++ b/src/SFML/Window/GlContext.cpp @@ -29,7 +29,7 @@ #include #include -#include +#include #include @@ -897,7 +897,7 @@ void GlContext::initialize(const ContextSettings& requestedSettings) if (!glGetIntegervFunc || !glGetErrorFunc || !glGetStringFunc || !glEnableFunc || !glIsEnabledFunc) { - err() << "Could not load necessary function to initialize OpenGL context" << std::endl; + log("Could not load necessary function to initialize OpenGL context"); return; } @@ -949,13 +949,12 @@ void GlContext::initialize(const ContextSettings& requestedSettings) !parseVersionString(version, "OpenGL ES ", m_settings.majorVersion, m_settings.minorVersion) && !parseVersionString(version, "", m_settings.majorVersion, m_settings.minorVersion)) { - err() << "Unable to parse OpenGL version string: " << std::quoted(version) << ", defaulting to 1.1" - << std::endl; + log("Unable to parse OpenGL version string: ", std::quoted(version), ", defaulting to 1.1"); } } else { - err() << "Unable to retrieve OpenGL version string, defaulting to 1.1" << std::endl; + log("Unable to retrieve OpenGL version string, defaulting to 1.1"); } } @@ -1038,7 +1037,7 @@ void GlContext::initialize(const ContextSettings& requestedSettings) // Check to see if the enable was successful if (glIsEnabledFunc(GL_FRAMEBUFFER_SRGB) == GL_FALSE) { - err() << "Warning: Failed to enable GL_FRAMEBUFFER_SRGB" << std::endl; + log("Warning: Failed to enable GL_FRAMEBUFFER_SRGB"); m_settings.sRgbCapable = false; } } @@ -1061,19 +1060,44 @@ void GlContext::checkSettings(const ContextSettings& requestedSettings) const (m_settings.antialiasingLevel < requestedSettings.antialiasingLevel) || (m_settings.depthBits < requestedSettings.depthBits) || (!m_settings.sRgbCapable && requestedSettings.sRgbCapable)) { - err() << "Warning: The created OpenGL context does not fully meet the settings that were requested" << '\n' - << "Requested: version = " << requestedSettings.majorVersion << "." << requestedSettings.minorVersion - << " ; depth bits = " << requestedSettings.depthBits << " ; stencil bits = " << requestedSettings.stencilBits - << " ; AA level = " << requestedSettings.antialiasingLevel << std::boolalpha - << " ; core = " << ((requestedSettings.attributeFlags & ContextSettings::Core) != 0) - << " ; debug = " << ((requestedSettings.attributeFlags & ContextSettings::Debug) != 0) - << " ; sRGB = " << requestedSettings.sRgbCapable << std::noboolalpha << '\n' - << "Created: version = " << m_settings.majorVersion << "." << m_settings.minorVersion - << " ; depth bits = " << m_settings.depthBits << " ; stencil bits = " << m_settings.stencilBits - << " ; AA level = " << m_settings.antialiasingLevel << std::boolalpha - << " ; core = " << ((m_settings.attributeFlags & ContextSettings::Core) != 0) - << " ; debug = " << ((m_settings.attributeFlags & ContextSettings::Debug) != 0) - << " ; sRGB = " << m_settings.sRgbCapable << std::noboolalpha << std::endl; + log("Warning: The created OpenGL context does not fully meet the settings that were requested\n", + "Requested: version = ", + requestedSettings.majorVersion, + ".", + requestedSettings.minorVersion, + " ; depth bits = ", + requestedSettings.depthBits, + " ; stencil bits = ", + requestedSettings.stencilBits, + " ; AA level = ", + requestedSettings.antialiasingLevel, + std::boolalpha, + " ; core = ", + ((requestedSettings.attributeFlags & ContextSettings::Core) != 0), + " ; debug = ", + ((requestedSettings.attributeFlags & ContextSettings::Debug) != 0), + " ; sRGB = ", + requestedSettings.sRgbCapable, + std::noboolalpha, + '\n', + "Created: version = ", + m_settings.majorVersion, + ".", + m_settings.minorVersion, + " ; depth bits = ", + m_settings.depthBits, + " ; stencil bits = ", + m_settings.stencilBits, + " ; AA level = ", + m_settings.antialiasingLevel, + std::boolalpha, + " ; core = ", + ((m_settings.attributeFlags & ContextSettings::Core) != 0), + " ; debug = ", + ((m_settings.attributeFlags & ContextSettings::Debug) != 0), + " ; sRGB = ", + m_settings.sRgbCapable, + std::noboolalpha); } } diff --git a/src/SFML/Window/SensorManager.cpp b/src/SFML/Window/SensorManager.cpp index 310282a96..15d53df21 100644 --- a/src/SFML/Window/SensorManager.cpp +++ b/src/SFML/Window/SensorManager.cpp @@ -27,7 +27,7 @@ //////////////////////////////////////////////////////////// #include -#include +#include #include @@ -59,8 +59,7 @@ void SensorManager::setEnabled(Sensor::Type sensor, bool enabled) } else { - err() << "Warning: trying to enable a sensor that is not available (call Sensor::isAvailable to check it)" - << std::endl; + log("Warning: trying to enable a sensor that is not available (call Sensor::isAvailable to check it)"); } } @@ -115,7 +114,7 @@ SensorManager::SensorManager() else { m_sensors[sensor].available = false; - err() << "Warning: sensor " << i << " failed to open, will not be available" << std::endl; + log("Warning: sensor ", i, " failed to open, will not be available"); } } } diff --git a/src/SFML/Window/Window.cpp b/src/SFML/Window/Window.cpp index 6d7abe6b1..eec82e709 100644 --- a/src/SFML/Window/Window.cpp +++ b/src/SFML/Window/Window.cpp @@ -29,7 +29,7 @@ #include #include -#include +#include #include #include @@ -164,7 +164,7 @@ bool Window::setActive(bool active) const return true; } - err() << "Failed to activate the window's context" << std::endl; + priv::log("Failed to activate the window's context"); return false; } @@ -201,7 +201,7 @@ void Window::initialize() // Activate the window if (!setActive()) { - err() << "Failed to set window as active during initialization" << std::endl; + priv::log("Failed to set window as active during initialization"); } WindowBase::initialize(); diff --git a/src/SFML/Window/WindowImpl.cpp b/src/SFML/Window/WindowImpl.cpp index 67317a4c6..7c1f6d2e0 100644 --- a/src/SFML/Window/WindowImpl.cpp +++ b/src/SFML/Window/WindowImpl.cpp @@ -31,7 +31,7 @@ #include #include -#include +#include #include #include @@ -126,17 +126,22 @@ std::unique_ptr WindowImpl::create( // Make sure there's not already a fullscreen window (only one is allowed) if (WindowImplImpl::fullscreenWindow != nullptr) { - err() << "Creating two fullscreen windows is not allowed, switching to windowed mode" << std::endl; + log("Creating two fullscreen windows is not allowed, switching to windowed mode"); state = State::Windowed; } // Make sure that the chosen video mode is compatible else if (!mode.isValid()) { - err() << "The requested video mode is not available, switching to a valid mode" << std::endl; + log("The requested video mode is not available, switching to a valid mode"); assert(!VideoMode::getFullscreenModes().empty() && "No video modes available"); mode = VideoMode::getFullscreenModes()[0]; - err() << " VideoMode: { size: { " << mode.size.x << ", " << mode.size.y - << " }, bitsPerPixel: " << mode.bitsPerPixel << " }" << std::endl; + log(" VideoMode: { size: { ", + mode.size.x, + ", ", + mode.size.y, + " }, bitsPerPixel: ", + mode.bitsPerPixel, + " }"); } } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 563d6d6c2..1e5974bd2 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -54,7 +54,6 @@ set(SYSTEM_SRC System/Angle.test.cpp System/Clock.test.cpp System/Config.test.cpp - System/Err.test.cpp System/FileInputStream.test.cpp System/MemoryInputStream.test.cpp System/Sleep.test.cpp @@ -76,6 +75,7 @@ set(WINDOW_SRC Window/Context.test.cpp Window/ContextSettings.test.cpp Window/Cursor.test.cpp + Window/Err.test.cpp # Tested alongside Window module because the System module performs no logging Window/Event.test.cpp Window/GlResource.test.cpp Window/Joystick.test.cpp diff --git a/test/System/Err.test.cpp b/test/Window/Err.test.cpp similarity index 59% rename from test/System/Err.test.cpp rename to test/Window/Err.test.cpp index 2735cc391..d71dceb03 100644 --- a/test/System/Err.test.cpp +++ b/test/Window/Err.test.cpp @@ -1,8 +1,13 @@ #include +// Other 1st party headers +#include + #include +#include #include +#include TEST_CASE("[System] sf::err") { @@ -38,4 +43,26 @@ TEST_CASE("[System] sf::err") sf::err().rdbuf(defaultStreamBuffer); CHECK(sf::err().rdbuf() == defaultStreamBuffer); } + + SECTION("Log errors") + { + using namespace std::string_literals; + using namespace std::string_view_literals; + + const std::stringstream stream; + sf::setErrorBuffer(stream.rdbuf()); + + // Produce logs concurrently from multiple threads + std::array threads{std::thread([] { (void)sf::Cursor::loadFromPixels(nullptr, {}, {}); }), + std::thread([] { (void)sf::Cursor::loadFromPixels(nullptr, {}, {}); }), + std::thread([] { (void)sf::Cursor::loadFromPixels(nullptr, {}, {}); })}; + for (auto& thread : threads) + thread.join(); + + // Ensure all messages come through in their entirety without being interleaved + CHECK(stream.str() == + "Failed to load cursor from pixels (invalid arguments)\n" + "Failed to load cursor from pixels (invalid arguments)\n" + "Failed to load cursor from pixels (invalid arguments)\n"); + } }