Remove deprecated fmt::localtime API and update fmt to 12.0.0 #1973
Conversation
WalkthroughCentralized timestamp formatting by adding VersionInfo::formatTimestamp(format) which converts the stored timestamp to local time and returns a preformatted string; callers in the About dialog and main JSON output now call this method instead of using inline fmt::localtime. Unit tests added for the new formatter. Changes
Sequence Diagram(s)sequenceDiagram
participant VersionInfo
participant GUI as AboutDialog
participant Main as main.cc
participant Clipboard
participant JSON
Note over VersionInfo: new API: formatTimestamp(format)
GUI->>VersionInfo: formatTimestamp("{:%Y-%m-%d %H:%M:%S}")
VersionInfo-->>GUI: "2025-01-18 02:27:53"
GUI->>Clipboard: copy(preformatted timestamp + other fields)
GUI->>GUI: display(preformatted timestamp)
Main->>VersionInfo: formatTimestamp("{:%Y-%m-%d %H:%M:%S}")
VersionInfo-->>Main: "2025-01-18 02:27:53"
Main->>JSON: insert(timestamp = preformatted string)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/main.cc (1)
245-249: Bug: memcard2 CLI arg assigned from memcard1 variable
SettingMcd2()is set fromargPath1.value()instead ofargPath2.value(). This breaks the--memcard2flag.Apply this fix:
- if (argPath2.has_value()) emuSettings.get<PCSX::Emulator::SettingMcd2>() = argPath1.value(); + if (argPath2.has_value()) emuSettings.get<PCSX::Emulator::SettingMcd2>() = argPath2.value();src/support/version.cc (1)
278-281: Wrong exception type: std::runtime_exception → std::runtime_error
std::runtime_exceptiondoesn't exist and will fail to compile on unsupported platforms.Apply this fix:
- throw std::runtime_exception("No platform support for updates"); + throw std::runtime_error("No platform support for updates");
🧹 Nitpick comments (3)
src/main/main.cc (1)
219-221: Centralized timestamp formatting (fmt12-safe) looks goodSwitching to
version.formatTimestamp("{:%Y-%m-%d %H:%M:%S}")removes the deprecatedfmt::localtimeusage and keeps output stable. Nice.Optional: consider emitting JSON numeric for "timestamp" (unquoted) in a future change; keep as-is if consumers rely on string.
src/support/version.h (1)
63-63: API polish: take std::string_viewAvoid needless allocation by accepting
std::string_view; callers can still pass literals andstd::string.Proposed change:
- std::string formatTimestamp(const std::string& format) const; + std::string formatTimestamp(std::string_view format) const;Add
#include <string_view>in this header.src/support/version.cc (1)
71-75: Timezone conversion: add robust fallback for environments without tzdb
std::chrono::current_zone()->to_local(...)may throw on platforms lacking the timezone database or with stripped tzdata. Consider a safe fallback tostd::localtimeto keep formatting working rather than propagating exceptions.Example:
std::string PCSX::VersionInfo::formatTimestamp(const std::string& format) const { - auto timepoint = std::chrono::system_clock::from_time_t(timestamp); - auto local = std::chrono::current_zone()->to_local(timepoint); - return fmt::format(fmt::runtime(format), floor<std::chrono::seconds>(local)); + auto tp = std::chrono::system_clock::from_time_t(timestamp); + try { + auto local = std::chrono::current_zone()->to_local(tp); + return fmt::format(fmt::runtime(format), std::chrono::floor<std::chrono::seconds>(local)); + } catch (...) { + // Fallback: thread-unsafe but acceptable here; copy the result. + std::tm tm_copy{}; + if (auto* tm_ptr = std::localtime(×tamp)) tm_copy = *tm_ptr; + return fmt::format(fmt::runtime(format), tm_copy); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/gui/gui.cc(1 hunks)src/main/main.cc(1 hunks)src/support/version.cc(2 hunks)src/support/version.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/support/version.h (1)
src/support/version.cc (2)
formatTimestamp(71-75)formatTimestamp(71-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
🔇 Additional comments (2)
src/support/version.cc (1)
30-33: Includes are appropriate for chrono/fmt formattingAdding
<chrono>,<fmt/chrono.h>, and<fmt/format.h>is correct for the new approach.src/gui/gui.cc (1)
2415-2438: Good: precompute timestamp once and reuse; removes deprecated API usageThis centralizes formatting via
VersionInfo::formatTimestampand avoids repeated conversions; clipboard and UI stay consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/support/version.cc(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
tests/support/version.cc
[error] 20-20: 'support/version.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: build-openbios
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
|
5f47854 takes the AI suggestion and removes the TZ assumption in the tests, but I don't love how it essentially reimplements the function in order to handle it. I'm open to suggestions on that. |
njfox
changed the title
|
fwiw, I needed this patch to get pcsx-redux to compile again on Arch Linux (using the |
2 participants
Fixes #1969
fmt::localtimewas deprecated and finally removed in fmt 12, which was pushed to Arch Linux recently, and this caused build failures and runtime errors when linked against the system fmt library. This PR removes the deprecated function and replaces it with a newformatTimestamp()function inVersionInfothat formats the timestamp based on a format string argument passed by the caller.Note that these changes are incompatible with fmt10, so we needed to update the submodule as well.