Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a thread-safe 'sf::Err' #3010

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vittorioromeo
Copy link
Member

Alternative design to #3008

@ChrisThrasher
Copy link
Member

ChrisThrasher commented May 16, 2024

I have a number of thoughts on error logging I wanted to share.

  • sf::err in its current form is part of the public API. I'd prefer to demote it to an implementation detail before v3.0.0 ships and we lose the ability to do that. You and I agree here. The only public API related to logging should be setting the destination stream buffer.
  • Users lack the ability to synchronize writes to sf::err so it's our responsibility to fix that.
  • I'm open to a std::print-like interface for error logging but don't necessarily hate the ostream-style status quo.
  • I'd like to see a specific named function for redirecting the output location for SFML's error logs.
  • I'd rather not bother with a rewrite such as this PR if and only if you plan on doing another std::print-style rewrite in the future. If we're going to rewrite all of sf::err then I want that to be more or the less final form it ships in for v3.0.0. I don't want to do two rewrites.

Here's what I picture as my ideal solution at this point in time. Opinions prone to change as I think more about this.

  • Expose one free function for setting the destination stream buffer
  • Define a free function with a std::print-like interface in a private header that simply uses a mutex to ensure synchronized writes to the underlying stream buffer. This ensures the stream buffer itself doesn't have to be thread safe, enabling the use of something like std::ostringstream as the underlying buffer.

In user code:

std::ostreamstring errorLog;
sf::setErrorLogBuffer(errorLog.rdbuf());

sf::Texture texture
if (!texture.loadFromFile("path/to/texture.png")) {
    std::cerr << errorLog.string(); // "Failed to load image..."
    return EXIT_FAILURE;
}

In SFML itself:

    else
    {
        // Error, failed to load the image
        logError("Failed to load image\n{}\nReason: {}", formatDebugPathInfo(filename), stbi_failure_reason());
        return false;
    }

How complicated is existed sf::err usage?

Scrolling through all uses of sf::err(), the vast majority simple stream in a string literal then std::endl so those use cases are trivia to support. In some cases two strings are appended. In rarer cases some variables are stringified into a larger sentence to provide useful debug output.

I only found one place where <iomanip> functions were used. I mention that because if we heavily used <iomanip> in formatting our error messages then it would require supporting more complicated std::format-style formatting options which would make such a transition more difficult. We may get away with only supporting {} in our format strings rather than more complicated things like {:>4.1f} which would pose a higher development cost. If we only have to support {} default formatting then I bet we can do that no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Discussion
Development

Successfully merging this pull request may close these issues.

None yet

3 participants