Match SPDLOG_CONSTEXPR_FUNC to FMT_CONSTEXPR

[kkraus14]

Fixes #2856.

PR based on #2858 and #2859.

Fixes the issue where a constexpr function in spdlog may call a non-constexpr function in fmt because FMT_CONSTEXPR is not defined or is defined differently for a given compiler.

Ideally, a test could be added of building a .cu file using nvcc and add that to CI to prevent nvcc breakages moving forward.

[kkraus14]

An alternative approach that might work here is to reuse FMT_CONSTEXPR with something like:

#ifdef FMT_CONSTEXPR
    #define SPDLOG_CONSTEXPR_FUNC FMT_CONSTEXPR
#else
    #if __cplusplus >= 201402L
        #define SPDLOG_CONSTEXPR_FUNC constexpr
    #else
        #define SPDLOG_CONSTEXPR_FUNC inline
    #endif
#endif

This may be friendlier to when we're using std::format as opposed to fmt as well.

[gabime]

An alternative approach that might work here is to reuse FMT_CONSTEXPR with something like

Agreed. Seems cleaner and to the point (use same constexpr as fmt).

[kkraus14]

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

[kkraus14]

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.

[gabime]

Why would it be empty?

[kkraus14]

It gets defined as empty for some reason:

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

Line 106 in cedfeeb

# define FMT_CONSTEXPR

[gabime]

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

[kkraus14]

Updated based on that

[kkraus14]

Looks like without the inline in C++11 (in C++14 and newer it would be constexpr) we hit some ODR violations from the header being included in multiple translation units that then get linked into a single artifact.

[gabime]

So I guess we need to use your first suggestion after all.

[kkraus14]

Reverted the change where I think this should be good to go. Thanks for your review and feedback @gabime!

[gabime]

Thanks @kkraus14