Skip to content

std::format instead of snprintf in logger.cpp#4767

Open
yotam-medini wants to merge 6 commits intocanonical:mainfrom
yotam-medini:stdfmt-4660-logger
Open

std::format instead of snprintf in logger.cpp#4767
yotam-medini wants to merge 6 commits intocanonical:mainfrom
yotam-medini:stdfmt-4660-logger

Conversation

@yotam-medini
Copy link
Contributor

Closes nothing

Related: #4660

Related to #4736

What's new?

std::format instead of snprintf in logger.cpp

How to test

Checklist

  • Tests added and pass
  • Adequate documentation added
  • (optional) Added Screenshots or videos

@yotam-medini yotam-medini requested a review from a team as a code owner March 11, 2026 14:39
@Saviq Saviq requested a review from Copilot March 11, 2026 16:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Mir’s core logger timestamp formatting to use C++ std::format/std::chrono instead of snprintf-based char buffers, aligning with the direction in issue #4660.

Changes:

  • Replace manual timespec/strftime/snprintf timestamp construction with std::chrono + std::format.
  • Add a fallback timestamp path when time zone formatting via zoned_time/current_zone() fails.
  • Build a preformatted "[timestamp] <severity>" prefix using std::format_to_n.

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

I know this idea originates from one of my comments, but I don't think that should be applied without regard to context.

Specifically, I'm not sure the change is well motivated here. The proposed code is harder to follow and adds complexity to widely used functionality. Is there anything it makes better?

Revert to UTC if current_zone() throws exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants