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

[core] New implementation for the logging/formatting system #2955

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

This reimplements the logging system by replacing the iostream-based system with a custom class that does formatting through snprintf. The use of std::ostringstream has been replaced with fmt::obufstream, which should do the same string building.

Other changes, beside replacement of state tags for formatting with the use of sfmt, are explicit type conversions limited to those that can be handled by snprintf.

srtcore/sfmt.h Fixed Show fixed Hide fixed
srtcore/logging.h Dismissed Show dismissed Hide dismissed
srtcore/sfmt.h Fixed Show fixed Hide fixed
srtcore/sfmt.h Fixed Show fixed Hide fixed
srtcore/utilities.h Fixed Show fixed Hide fixed
srtcore/logging.h Dismissed Show dismissed Hide dismissed
@ethouris ethouris marked this pull request as ready for review June 19, 2024 13:04
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Jun 19, 2024
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Jun 19, 2024
srtcore/sfmt.h Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a unique name to avoid conflicts with the fmt library.
For example, srt_format.h.
The same for namespaces used. The main one must be srt. Then maybe again not fmt but something more unique, unless fmt library can be used instead if enabled in CMake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't actually, this is distributed in my fork of the {fmt} library and there are no conflicting names there (and its license is PD, so it can be freely copied). Although I understand that to really avoid any potential conflicts in the future it should use different naming. I would keep the "sfmt" name at least as a part of the filename. As for the namespace, I think it may simply go whole into srt namespace, I think there are no name conflicts against it.

srtcore/sfmt.h Outdated
@@ -0,0 +1,1047 @@
// Formatting library for C++ - C++03 compat version of on-demand tagged format API.
//
// Copyright (c) 2024 - present, Mikołaj Małecki
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright - Haivision. Authors: Mikołaj Małecki

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm not sure about that. Although it should be mentioned that it is copied from an open-source library distributed with PD license.

const internal::form_memory_buffer<2> seol ("\n");

// Another manipulator. You can add yourself others the same way.
const struct os_flush_manip {} sflush;

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable sflush is never read.
void open(const std::string& name, const std::string& mode = "")
{
if (mode == "")
in = std::fopen(name.c_str(), "w");

Check failure

Code scanning / CodeQL

File created without restricting permissions High

A file may be created here with mode 0666, which would make it world-writable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants