Skip to content

Replace non-reentrant localtime with localtime_r in databaseConnection.cpp #13

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.

Sign up for GitHub

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

Merged
mccaffers merged 2 commits into from
Apr 6, 2026
Merged

Replace non-reentrant localtime with localtime_r in databaseConnection.cpp #13

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98b485d
Initial plan
Copilot Apr 6, 2026
762fefc
Replace non-reentrant localtime with localtime_r, zero-init struct tm…
Copilot Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion source/databaseConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ void DatabaseConnection::printResults(const std::vector<PriceData>& results) con
for (const auto& data : results) {
// Convert timestamp back to string for display
auto time_t = std::chrono::system_clock::to_time_t(data.timestamp);
auto tm = *std::localtime(&time_t);
struct tm tm = {};
if (localtime_r(&time_t, &tm) == nullptr) {
std::cerr << "Error: failed to convert timestamp" << std::endl;
Comment on lines +64 to +66
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

localtime_r is a POSIX API and is not available under MSVC/Windows builds. Since the project’s CMakeLists explicitly supports MSVC, this change can break Windows compilation. Consider adding a small platform abstraction (e.g., #if defined(_WIN32) use localtime_s(&tm, &time_t); otherwise localtime_r) and qualifying the call (::localtime_r) with the appropriate <ctime>/<time.h> include so the declaration is guaranteed to be visible.

Copilot uses AI. Check for mistakes.
continue;
}
Comment on lines +65 to +68
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

This change introduces a new failure branch (localtime_r(...) == nullptr) that alters output behavior (skips entries). The repository has XCTest coverage for DatabaseConnection (e.g., tests/db.mm), but there’s no test exercising printResults timestamp formatting or the new error path. Consider extracting the timestamp-to-string formatting into a helper that can be unit tested (including the failure case) without relying on stdout/stderr capture.

Copilot uses AI. Check for mistakes.
std::stringstream ss;
ss << std::put_time(&tm, "%Y-%m-%d %H:%M:%S");

Expand Down
Loading