sstring: inherit publicly from string_view formatter #1855

avikivity · 2023-10-01T16:31:17Z

fmt 10 detects if a formatter provides some method (set_debug_format()),
but the detection fails if the method is private and compilation breaks
on clang [1].

Work around the clang bug by inheriting publicly.

[1] llvm/llvm-project#68849

A test case reproducing the problem is included (thanks
Kefu Chai kefu.chai@scylladb.com).

tchaikov · 2023-10-07T07:06:30Z

include/seastar/core/sstring.hh

@@ -883,7 +883,7 @@ std::ostream& operator<<(std::ostream& os, const std::unordered_map<Key, T, Hash
 SEASTAR_MODULE_EXPORT
 template <typename char_type, typename Size, Size max_size, bool NulTerminate>
 struct fmt::formatter<seastar::basic_sstring<char_type, Size, max_size, NulTerminate>>
-    : private fmt::formatter<std::basic_string_view<char_type>> {
+    : public fmt::formatter<std::basic_string_view<char_type>> {


i understand that ranges formatter uses maybe_set_debug_format() to optionally enables the debugging facilities when the formatting specifier is "?". and this helper function is specialized for the case where set_debug_format() is callable. see https://github.com/fmtlib/fmt/blob/f76603f21ea75bcd8d0007d7599cac75862b13b9/include/fmt/ranges.h#L287-L293

after reading the related code in fmtlib, i think our formatter should be correct, if it fails the build, it should be a bug in the compiler or fmtlib. but i cannot reproduce this failure with following patch:

diff --git a/cooking_recipe.cmake b/cooking_recipe.cmake index 32e2b3a1..92e13efe 100644 --- a/cooking_recipe.cmake +++ b/cooking_recipe.cmake @@ -312,8 +312,7 @@ cooking_ingredient (dpdk cooking_ingredient (fmt EXTERNAL_PROJECT_ARGS - URL https://github.com/fmtlib/fmt/archive/9.1.0.tar.gz - URL_MD5 21fac48cae8f3b4a5783ae06b443973a + URL https://github.com/fmtlib/fmt/archive/10.1.1.tar.gz CMAKE_ARGS -DFMT_DOC=OFF -DFMT_TEST=OFF) diff --git a/tests/unit/sstring_test.cc b/tests/unit/sstring_test.cc index 2cc78383..bcd6593e 100644 --- a/tests/unit/sstring_test.cc +++ b/tests/unit/sstring_test.cc @@ -24,6 +24,7 @@ #include <boost/test/unit_test.hpp> #include <seastar/core/sstring.hh> #include <list> +#include <fmt/ranges.h> using namespace std::literals; using namespace seastar; @@ -306,3 +307,13 @@ BOOST_AUTO_TEST_CASE(test_compares_left_hand_not_string) { BOOST_REQUIRE(std::string("a") < sstring("b")); #endif } + +#if FMT_VERSION >= 90000 + +BOOST_AUTO_TEST_CASE(test_range_fmt) { + std::ignore = fmt::format("{}", sstring{"hello"}); + std::vector<sstring> strings; + std::ignore = fmt::format("{}", strings); +} + +#endif

i tested with clang 16.0.6, and fmtlib 10.1.1. the building system is generated using

$ ./configure.py --cook fmt --compiler clang++ --c-compiler clang --mode debug --disable-dpdk

I will try to extract a reproducer.

FAILED: build/dev/test/unit/row_cache_stress_test.o clang++ -MD -MT build/dev/test/unit/row_cache_stress_test.o -MF build/dev/test/unit/row_cache_stress_test.o.d -I/home/avi/scylla/seastar/include -I/home/avi/scylla/build/dev/seastar/gen/include -U_FORTIFY_SOURCE -Werror=unused-result -fstack-clash-protection -DSEASTAR_API_LEVEL=7 -DSEASTAR_BUILD_SHARED_LIBS -DSEASTAR_SSTRING -DSEASTAR_ENABLE_ALLOC_FAILURE_INJECTION -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_LOGGER_TYPE_STDOUT -DSEASTAR_TYPE_ERASE_MORE -DFMT_SHARED -I/usr/include/p11-kit-1 -ffile-prefix-map=/home/avi/scylla=. -march=westmere -DDEVEL -DSEASTAR_ENABLE_ALLOC_FAILURE_INJECTION -DSCYLLA_ENABLE_ERROR_INJECTION -O2 -DSCYLLA_BUILD_MODE=dev -iquote. -iquote build/dev/gen --std=gnu++20 -ffile-prefix-map=/home/avi/scylla=. -march=westmere -DBOOST_TEST_DYN_LINK -DNOMINMAX -DNOMINMAX -fvisibility=hidden -Wall -Werror -Wimplicit-fallthrough -Wno-mismatched-tags -Wno-c++11-narrowing -Wno-overloaded-virtual -Wno-unused-command-line-argument -Wno-unsupported-friend -Wno-implicit-int-float-conversion -Wno-psabi -Wno-narrowing -Wno-error=deprecated-declarations -DXXH_PRIVATE_API -DSEASTAR_TESTING_MAIN -c -o build/dev/test/unit/row_cache_stress_test.o test/unit/row_cache_stress_test.cc In file included from test/unit/row_cache_stress_test.cc:22: /usr/include/fmt/std.h:122:7: error: 'set_debug_format' is a private member of 'fmt::formatter<fmt::basic_string_view<char>>' 122 | u.set_debug_format(set); | ~~^~~~~~~~~~~~~~~~ /usr/include/fmt/std.h:130:5: note: in instantiation of function template specialization 'fmt::formatter<std::optional<seastar::basic_sstring<char, unsigned int, 15>>>::maybe_set_debug_format<fmt::formatter<seastar::basic_sstring<char, unsigned int, 15>>>' requested here 130 | maybe_set_debug_format(underlying_, true); | ^ /usr/include/fmt/core.h:1297:28: note: in instantiation of function template specialization 'fmt::formatter<std::optional<seastar::basic_sstring<char, unsigned int, 15>>>::parse<fmt::basic_format_parse_context<char>>' requested here 1297 | parse_ctx.advance_to(f.parse(parse_ctx)); | ^ /usr/include/fmt/core.h:1283:21: note: in instantiation of function template specialization 'fmt::detail::value<fmt::basic_format_context<fmt::appender, char>>::format_custom_arg<std::optional<seastar::basic_sstring<char, unsigned int, 15>>, fmt::formatter<std::optional<seastar::basic_sstring<char, unsigned int, 15>>>>' requested here 1283 | custom.format = format_custom_arg< | ^ /home/avi/scylla/seastar/include/seastar/core/print.hh:145:10: note: in instantiation of function template specialization 'fmt::format_to<fmt::appender, unsigned long &, std::optional<seastar::basic_sstring<char, unsigned int, 15>> &, seastar::basic_sstring<char, unsigned int, 15> &, 0>' requested here 145 | fmt::format_to(fmt::appender(out), fmt::runtime(fmt), std::forward<A>(a)...); | ^ test/unit/row_cache_stress_test.cc:228:38: note: in instantiation of function template specialization 'seastar::format<unsigned long &, std::optional<seastar::basic_sstring<char, unsigned int, 15>> &, seastar::basic_sstring<char, unsigned int, 15> &>' requested here 228 | throw std::runtime_error(format("Saw values from two different writes in partition {:d}: {} and {}", _key, _value, value)); | ^ /home/avi/scylla/seastar/include/seastar/core/sstring.hh:886:7: note: constrained by private inheritance here 886 | : private fmt::formatter<std::basic_string_view<char_type>> { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/fmt/core.h:2715:22: note: member is declared here 2715 | FMT_CONSTEXPR void set_debug_format(bool set = true) { | ^ 1 error generated.

Working reproducer:

diff --git a/tests/unit/sstring_test.cc b/tests/unit/sstring_test.cc index 2cc7838389..7b94d95b02 100644 --- a/tests/unit/sstring_test.cc +++ b/tests/unit/sstring_test.cc @@ -22,10 +22,12 @@ #define BOOST_TEST_MODULE core #include <boost/test/unit_test.hpp> #include <seastar/core/sstring.hh> #include <list> +#include <fmt/ranges.h> +#include <fmt/std.h> using namespace std::literals; using namespace seastar; BOOST_AUTO_TEST_CASE(test_make_sstring) { @@ -304,5 +306,15 @@ BOOST_AUTO_TEST_CASE(test_compares_left_hand_not_string) { #ifdef __cpp_lib_three_way_comparison BOOST_REQUIRE("a" < sstring("b")); BOOST_REQUIRE(std::string("a") < sstring("b")); #endif } + +#if FMT_VERSION >= 90000 + +BOOST_AUTO_TEST_CASE(test_range_fmt) { + std::ignore = fmt::format("{}", std::optional(sstring{"hello"})); + std::vector<sstring> strings; + std::ignore = fmt::format("{}", strings); +} + +#endif diff --git a/cooking_recipe.cmake b/cooking_recipe.cmake index 32e2b3a17a..1789664d93 100644 --- a/cooking_recipe.cmake +++ b/cooking_recipe.cmake @@ -310,12 +310,11 @@ cooking_ingredient (dpdk INSTALL_COMMAND ${Ninja_EXECUTABLE} -C <BINARY_DIR> install) cooking_ingredient (fmt EXTERNAL_PROJECT_ARGS - URL https://github.com/fmtlib/fmt/archive/9.1.0.tar.gz - URL_MD5 21fac48cae8f3b4a5783ae06b443973a + URL https://github.com/fmtlib/fmt/archive/10.1.1.tar.gz CMAKE_ARGS -DFMT_DOC=OFF -DFMT_TEST=OFF) cooking_ingredient (liburing

sstring works, but optional` failed.

i see. it is a clang bug. see https://godbolt.org/z/4GMdKb9en . still working on a minimal reproducer, so i can report it upstream.

filed llvm/llvm-project#68849

great, thanks.

avikivity · 2023-10-08T14:07:31Z

v2: add test case

tchaikov · 2023-10-11T06:59:11Z

tests/unit/sstring_test.cc

+
+#if FMT_VERSION >= 90000
+
+BOOST_AUTO_TEST_CASE(test_range_fmt) {


Suggested change

BOOST_AUTO_TEST_CASE(test_range_fmt) {

BOOST_AUTO_TEST_CASE(test_fmt) {

as it tests both range and std::optional<sstring>.

tchaikov · 2023-10-11T07:26:55Z

include/seastar/core/sstring.hh

@@ -883,7 +883,7 @@ std::ostream& operator<<(std::ostream& os, const std::unordered_map<Key, T, Hash
 SEASTAR_MODULE_EXPORT
 template <typename char_type, typename Size, Size max_size, bool NulTerminate>
 struct fmt::formatter<seastar::basic_sstring<char_type, Size, max_size, NulTerminate>>
-    : private fmt::formatter<std::basic_string_view<char_type>> {
+    : public fmt::formatter<std::basic_string_view<char_type>> {


i see. it is a clang bug. see https://godbolt.org/z/4GMdKb9en . still working on a minimal reproducer, so i can report it upstream.

tchaikov · 2023-10-11T07:33:14Z

include/seastar/core/sstring.hh

@@ -883,7 +883,7 @@ std::ostream& operator<<(std::ostream& os, const std::unordered_map<Key, T, Hash
 SEASTAR_MODULE_EXPORT
 template <typename char_type, typename Size, Size max_size, bool NulTerminate>
 struct fmt::formatter<seastar::basic_sstring<char_type, Size, max_size, NulTerminate>>
-    : private fmt::formatter<std::basic_string_view<char_type>> {
+    : public fmt::formatter<std::basic_string_view<char_type>> {


the reason why i chose to use private inheritance was to avoid the disputations based on Liskov principal (the IS-A principal). but if we need to workaround this compiler bug. i think i'd give up and just use the public inheritance, as it's simpler:

drop private.

drop using base::parse

optionally replace base with fmt::formatter<format_as_t>. as it is not reused.

I agree that private is better here, I switched to public to make it work, we can add a comment referencing the clang bug once we know what it is.

tchaikov · 2023-10-11T08:32:08Z

also, might want to s/strig_view/string_view/ in the title of the commit message.

tchaikov · 2023-10-11T08:39:50Z

tests/unit/sstring_test.cc

+#if FMT_VERSION >= 90000
+
+BOOST_AUTO_TEST_CASE(test_range_fmt) {
+    std::ignore = fmt::format("{}", std::optional(sstring{"hello"}));


might want to guard this very test with #if FMT_VERSION >= 100000, and keep the test for fmt/range in #if FMT_VERSION >= 90000. as the formatter for std::optional<> as introduced by fmtlib v10.0.0, see https://github.com/fmtlib/fmt/releases/tag/10.0.0.

this is why the CI testing is failing for this PR -- we only have access to fmt-devel 9.1.0 in the frozen toolchain docker container.

avikivity · 2023-10-17T11:15:00Z

v3:

fixed spelling in commitlog
reworded commitlog to point to clang bug
added comments referencing clang bug
only test std::optional formatting with fmt 10

tchaikov

lgtm. could you reconsider the 2nd item in https://github.com/scylladb/seastar/pull/1855/files#r1354315456 . as using base::parse is not necessary anymore.

avikivity · 2023-10-17T12:35:53Z

v5: drop now unnecessary using base::parse

tchaikov

thanks. still lgtm

tchaikov · 2023-10-18T03:38:45Z

just a nit: s/sstring_view/string_view/

fmt 10 detects if a formatter provides some method (set_debug_format()), but the detection fails if the method is private and compilation breaks on clang [1]. Work around the clang bug by inheriting publicly. [1] llvm/llvm-project#68849 A test case reproducing the problem is included (thanks Kefu Chai <kefu.chai@scylladb.com>).

avikivity · 2023-10-18T10:09:32Z

v6: s/s(string_view)/\1/

tchaikov

lgtm.

avikivity · 2023-10-19T08:53:25Z

@nyh please review

fmt 10 detects if a formatter provides some method (set_debug_format()), but the detection fails if the method is private and compilation breaks on clang [1]. Work around the clang bug by inheriting publicly. [1] llvm/llvm-project#68849 A test case reproducing the problem is included (thanks Kefu Chai <kefu.chai@scylladb.com>). Closes scylladb#1855

avikivity requested a review from tchaikov October 2, 2023 15:34

tchaikov reviewed Oct 7, 2023

View reviewed changes

avikivity force-pushed the sstring-formatter-not-private branch from 9d102dd to 5794968 Compare October 8, 2023 14:07

tchaikov reviewed Oct 11, 2023

View reviewed changes

avikivity force-pushed the sstring-formatter-not-private branch from 5794968 to e4cd64b Compare October 17, 2023 11:14

avikivity changed the title ~~sstring: inherit publicly from strig_view formatter~~ sstring: inherit publicly from sstring_view formatter Oct 17, 2023

tchaikov approved these changes Oct 17, 2023

View reviewed changes

avikivity force-pushed the sstring-formatter-not-private branch from e4cd64b to 6304e0e Compare October 17, 2023 12:45

tchaikov approved these changes Oct 17, 2023

View reviewed changes

avikivity changed the title ~~sstring: inherit publicly from sstring_view formatter~~ sstring: inherit publicly from string_view formatter Oct 18, 2023

avikivity force-pushed the sstring-formatter-not-private branch from 6304e0e to 2a4a7b1 Compare October 18, 2023 10:08

tchaikov approved these changes Oct 18, 2023

View reviewed changes

avikivity requested a review from nyh October 19, 2023 08:53

nyh approved these changes Oct 19, 2023

View reviewed changes

nyh closed this in e6a848b Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sstring: inherit publicly from string_view formatter #1855

sstring: inherit publicly from string_view formatter #1855

avikivity commented Oct 1, 2023 •

edited

Loading

tchaikov Oct 7, 2023

avikivity Oct 8, 2023

avikivity Oct 8, 2023

avikivity Oct 8, 2023

avikivity Oct 8, 2023

tchaikov Oct 11, 2023

tchaikov Oct 12, 2023

avikivity Oct 17, 2023

avikivity commented Oct 8, 2023

tchaikov Oct 11, 2023

tchaikov Oct 11, 2023

tchaikov Oct 11, 2023

avikivity Oct 11, 2023

tchaikov commented Oct 11, 2023

tchaikov Oct 11, 2023

avikivity Oct 17, 2023

avikivity commented Oct 17, 2023

tchaikov left a comment

avikivity commented Oct 17, 2023

tchaikov left a comment

tchaikov commented Oct 18, 2023

avikivity commented Oct 18, 2023

tchaikov left a comment

avikivity commented Oct 19, 2023


		#if FMT_VERSION >= 90000

		BOOST_AUTO_TEST_CASE(test_range_fmt) {

	BOOST_AUTO_TEST_CASE(test_range_fmt) {
	BOOST_AUTO_TEST_CASE(test_fmt) {

sstring: inherit publicly from string_view formatter #1855

sstring: inherit publicly from string_view formatter #1855

Conversation

avikivity commented Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avikivity commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avikivity commented Oct 17, 2023

tchaikov left a comment

Choose a reason for hiding this comment

avikivity commented Oct 17, 2023

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov commented Oct 18, 2023

avikivity commented Oct 18, 2023

tchaikov left a comment

Choose a reason for hiding this comment

avikivity commented Oct 19, 2023

avikivity commented Oct 1, 2023 •

edited

Loading