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

Missing test for __APPLE__ chooses buggy SunOS/Solaris workaround in utc_minutes_offset #3352

Closed
toh-ableton opened this issue Feb 28, 2025 · 2 comments

Comments

@toh-ableton
Copy link
Contributor

spdlog uses the function utc_minutes_offset in the z_formatter to format the '%z'-part of a format string (UTC-offset) when logging time. The function has three implementations, one of which is chosen at compile time via marcro defines (Windows, SunOS/Solaris, default):

#if defined(sun) || defined(__sun) || defined(_AIX) || \
(defined(__NEWLIB__) && !defined(__TM_GMTOFF)) || \
(!defined(_BSD_SOURCE) && !defined(_GNU_SOURCE))
// 'tm_gmtoff' field is BSD extension and it's missing on SunOS/Solaris

This test doesn't detect Apple platforms, which have had the tm_gmtoff-field at least since Mac OS X 10.0, and also doesn't detect POSIX.1-2024 conforming systems, which are also required to support tm_gmtoff.

On Apple and POSIX1.-2024 platforms, this has the unfortunate effect to use the SunOS/Solaris fallback, which doesn't compute the correct value if the passed value of tm isn't the current system time, i.e. localtime(::time()) (#3351).

I suggest to fix this by changing the test to something like

    #if defined(sun) || defined(__sun) || defined(_AIX) || \
        (defined(__NEWLIB__) && !defined(__TM_GMTOFF)) ||  \
        (!defined(__APPLE__) && !defined(_BSD_SOURCE) && !defined(_GNU_SOURCE) && \
            (!defined(_POSIX_VERSION) || (_POSIX_VERSION < 202405L)))
    // 'tm_gmtoff' field is BSD extension and it's missing on SunOS/Solaris
@toh-ableton toh-ableton changed the title Missing test for __APPLE__ chooses buggy SunOS/Solaris workaround in Missing test for __APPLE__ chooses buggy SunOS/Solaris workaround in utc_minutes_offset Feb 28, 2025
@gabime
Copy link
Owner

gabime commented Feb 28, 2025

Thanks for the report. Is there a test that can be added to tests/ to reproduce this?
PR is always welcome.

toh-ableton added a commit to toh-ableton/spdlog that referenced this issue Mar 6, 2025
…workaround)

Apple platforms have had the tm_gmtoff-field at least since Mac OS X 10.0,
as are POSIX.1-2024 conforming systems, which are also required to support
it.

This has the unfortunate effect to use the SunOS/Solaris fallback, which
doesn't compute the correct value if the passed value of tm isn't the
current system time, i.e. localtime(::time()) (gabime#3351).
@toh-ableton
Copy link
Contributor Author

Sure: #3353.

@gabime gabime closed this as completed in 9c58257 Mar 29, 2025
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

No branches or pull requests

2 participants