Incorrect FMT_USE_STRING_VIEW logic results in libstdc++ compilation errors

[harrism]

The following linked code in core.h results in compilation errors compiling with GCC (libstdc++) in our (C++14) library (https://github.com/rapidsai/cudf) which depends on spdlog, which bundles a version of fmt.

fmt/include/fmt/core.h

Lines 228 to 237 in 6cccdc2

	// libc++ supports string_view in pre-c++17.
	#if (FMT_HAS_INCLUDE(<string_view>) && \
	(__cplusplus > 201402L || defined(_LIBCPP_VERSION))) || \
	(defined(_MSVC_LANG) && _MSVC_LANG > 201402L && _MSC_VER >= 1910)
	# include <string_view>
	# define FMT_USE_STRING_VIEW
	#elif FMT_HAS_INCLUDE("experimental/string_view") && __cplusplus >= 201402L
	# include <experimental/string_view>
	# define FMT_USE_EXPERIMENTAL_STRING_VIEW
	#endif

Specifically, while libc++ (clang) provides string_view before C++17, libstdc++ does not, it provides experimental/string_view.

Thus, I believe the || on line 230 should be &&, and making this change fixes our compilation issues.

[harrism]

Note that I can't produce the compilation failure in fmt's tests on the same system. I can only repro in building https://github.com/rapidsai/cudf, which depends on https://github.com/rapidsai/rmm, which depends on https://github.com/gabime/spdlog, which bundles fmt. Nevertheless, I think it's clear from inspection that || is wrong in the linked code. You want to detect that libc++ is the std implementation and that compilation is later than C++14, because otherwise it passes as long as C++ > 14 even if std is libstdc++, which makes it fail to find a compatible string_view header.

[vitaut]

According to #686, libc++ supports string_view pre-C++17. If this is true then the check looks correct. In your case _LIBCPP_VERSION should be undefined since you are using libstdc++.

[vitaut]

You can see that the check is correct on godbolt:

• https://godbolt.org/z/6h53GW: libstdc++, no string_view
• https://godbolt.org/z/6v54sa: libc++, has string_view

[harrism]

Thanks for the clue. We depend on libcudac++, which is based on libc++, and I think including that before including fmt is causing _LIBCPP_VERSION to be defined...

That said, it seems like a more conservative check like the one in #1852 is still correct and may allow use cases like ours (that have parts of both standard libraries in effect) to work.

[vitaut]

Unfortunately the check in #1852 is not correct because std::string_view is available in libc++ pre C++17. You probably need a more cuda-specific workaround.

[vitaut]

Closed since it looks like an issue with libcudac++ but I'm open to a workaround that doesn't break string_view detection in normal libc++.

[harrism]

@vitaut thanks for your time and sorry for the noise. Looks like this is fixed in the latest libcudac++ and I should have a workaround until we are able to use the latest.

[vitaut]

No worries, glad it's fixed.