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.

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

Jump to bottom

Match SPDLOG_CONSTEXPR_FUNC to FMT_CONSTEXPR #2901

Merged
merged 5 commits into from
Oct 13, 2023
Merged

Match SPDLOG_CONSTEXPR_FUNC to FMT_CONSTEXPR #2901

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0ad647
Modify the condition of SPDLOG_CONSTEXPR_FUNC to match that of fmt
ShujianQian Aug 18, 2023
23c69f8
add SPDLOG_HAS_FEATURE macro to avoid redefining built-in, add commen…
kkraus14 Oct 11, 2023
2421b09
Update implementation to reuse FMT_CONSTEXPR where possible
kkraus14 Oct 11, 2023
1897f70
don't need to inline, simplify to always use FMT_CONSTEXPR if it's de…
kkraus14 Oct 12, 2023
fab780c
Revert "don't need to inline, simplify to always use FMT_CONSTEXPR if…
kkraus14 Oct 12, 2023
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
17 changes: 13 additions & 4 deletions include/spdlog/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,23 @@
#if defined(_MSC_VER) && (_MSC_VER < 1900)
#define SPDLOG_NOEXCEPT _NOEXCEPT
#define SPDLOG_CONSTEXPR
#define SPDLOG_CONSTEXPR_FUNC inline
#else
#define SPDLOG_NOEXCEPT noexcept
#define SPDLOG_CONSTEXPR constexpr
#if __cplusplus >= 201402L
#define SPDLOG_CONSTEXPR_FUNC constexpr
#endif

// If building with std::format, can just use constexpr, otherwise if building with fmt
// SPDLOG_CONSTEXPR_FUNC needs to be set the same as FMT_CONSTEXPR to avoid situations where
// a constexpr function in spdlog could end up calling a non-constexpr function in fmt
// depending on the compiler
// If fmt determines it can't use constexpr, we should inline the function instead
#ifdef SPDLOG_USE_STD_FORMAT
#define SPDLOG_CONSTEXPR_FUNC constexpr
#else // Being built with fmt
#if FMT_USE_CONSTEXPR
#define SPDLOG_CONSTEXPR_FUNC FMT_CONSTEXPR
#else
#define SPDLOG_CONSTEXPR_FUNC inline
#define SPDLOG_CONSTEXPR_FUNC inline
Comment on lines +81 to +84
Copy link
Contributor Author

@kkraus14 kkraus14 Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I needed to change this slightly to maintain the current behavior. If fmt determines it can't use constexpr then it defines FMT_CONSTEXPR as empty

spdlog/include/spdlog/fmt/bundled/core.h

Lines 92 to 107 in cedfeeb

// Check if relaxed C++14 constexpr is supported.
// GCC doesn't allow throw in constexpr until version 6 (bug 67371).
#ifndef FMT_USE_CONSTEXPR
# if (FMT_HAS_FEATURE(cxx_relaxed_constexpr) || FMT_MSC_VERSION >= 1912 || \
(FMT_GCC_VERSION >= 600 && FMT_CPLUSPLUS >= 201402L)) && \
!FMT_ICC_VERSION && !defined(__NVCC__)
# define FMT_USE_CONSTEXPR 1
# else
# define FMT_USE_CONSTEXPR 0
# endif
#endif
#if FMT_USE_CONSTEXPR
# define FMT_CONSTEXPR constexpr
#else
# define FMT_CONSTEXPR
#endif
while spdlog's current behavior is to define it as inline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably use some preprocessor magic to detect when FMT_CONSTEXPR is empty and use inline instead in those situations, but that adds some non-trivial complexity.

Copy link
Owner

Choose a reason for hiding this comment

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

Why would it be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets defined as empty for some reason:

spdlog/include/spdlog/fmt/bundled/core.h

Line 106 in cedfeeb

# define FMT_CONSTEXPR

Copy link
Owner

Choose a reason for hiding this comment

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

I think it won't matter for spdlog. It can be empty for spdlog as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated based on that

#endif
#endif

Expand Down