Please support fast_float 7.0.0

[musicinmybrain]

Version 7.0.0 of fast_float has just been released.

One major change is that char_format is no longer an enum, but an enum class with underlying type uint64_t.

This requires a bit of adaptation, something like this, which should be backwards-compatible with older fast_float releases:

diff --git a/src/scn/impl.cpp b/src/scn/impl.cpp
index aa0d334..2ad35ff 100644
--- a/src/scn/impl.cpp
+++ b/src/scn/impl.cpp
@@ -721,12 +721,14 @@ scan_expected<std::ptrdiff_t> fast_float_fallback(impl_init_data<CharT> data,
 struct fast_float_impl_base : impl_base {
     fast_float::chars_format get_flags() const
     {
-        unsigned format_flags{};
+        uint64_t format_flags{};
         if ((m_options & float_reader_base::allow_fixed) != 0) {
-            format_flags |= fast_float::fixed;
+            format_flags |=
+                static_cast<uint64_t>(fast_float::chars_format::fixed);
         }
         if ((m_options & float_reader_base::allow_scientific) != 0) {
-            format_flags |= fast_float::scientific;
+            format_flags |=
+                static_cast<uint64_t>(fast_float::chars_format::scientific);
         }

         return static_cast<fast_float::chars_format>(format_flags);

However, I still have a problem after making this change:

In file included from /home/ben/src/forks/scnlib/build/_deps/fast_float-src/include/fast_float/fast_float.h:57,
                 from /home/ben/src/forks/scnlib/benchmark/runtime/float/single.cpp:26:
/home/ben/src/forks/scnlib/build/_deps/fast_float-src/include/fast_float/parse_number.h: In instantiation of ‘fast_float::from_chars_result_t<UC> fast_float::from_chars(const UC*, const UC*, T&, int) [with T = long double; UC = char; <template-parameter-1-3> = int]’:
/home/ben/src/forks/scnlib/benchmark/runtime/float/single.cpp:176:42:   required from ‘void scan_float_single_fastfloat(benchmark::State&) [with Float = long double]’
  176 |         auto ret = fast_float::from_chars(s.it->data(),
      |                    ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
  177 |                                           s.it->data() + s.it->size(), f);
      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ben/src/forks/scnlib/benchmark/runtime/float/single.cpp:188:1:   required from here
 1539 |               #n "<" #__VA_ARGS__ ">", n<__VA_ARGS__>)))
      |                                                      ^
/home/ben/src/forks/scnlib/build/_deps/fast_float-src/include/fast_float/parse_number.h:326:38: error: static assertion failed: only integer types are supported
  326 |   static_assert(std::is_integral<T>::value, "only integer types are supported");
      |                                      ^~~~~
/home/ben/src/forks/scnlib/build/_deps/fast_float-src/include/fast_float/parse_number.h:326:38: note: ‘std::integral_constant<bool, false>::value’ evaluates to false

This error is coming from code that was added in fastfloat/fast_float@0bbba96 as part of fastfloat/fast_float#280, but it’s not immediately clear to me whether this is a regression in fast_float or whether it reflects something that ought to be changed in scnlib.

I’ll open a draft PR corresponding to the work described above.

[dalle]

The problem is that fast_float doesn't support long double. But the test is using long double:

BENCHMARK_TEMPLATE(scan_float_single_fastfloat, long double);

The error could be better though.

[dalle]

Regarding the get_flags() function. I think the following is a bit more slim, and backwards compatible;

     fast_float::chars_format get_flags() const
     {
        fast_float::chars_format format_flags{};
         if ((m_options & float_reader_base::allow_fixed) != 0) {
            format_flags |= fast_float::chars_format::fixed;
         }
         if ((m_options & float_reader_base::allow_scientific) != 0) {
            format_flags |= fast_float::chars_format::scientific;
         }
         return format_flags;
     }

[musicinmybrain]

Regarding the get_flags() function. I think the following is a bit more slim, and backwards compatible;

Yes, that’s much better. Thanks. I failed to notice that fast_float had implemented chars_format & operator|=(chars_format &lhs, chars_format rhs) noexcept. It makes sense that they did so, considering this enum class has “bit flags” semantics. I’ll amend the PR.

[musicinmybrain]

Regarding the get_flags() function. I think the following is a bit more slim, and backwards compatible;

Technically, this isn’t quite backwards-compatible, because we have changed the type of format_flags from unsigned to fast_float::char_format, and there is no default implementation of operator|= for plain enums either.

Trying this with fast_float 6.1.6:

/home/ben/src/forks/scnlib/src/scn/impl.cpp: In member function ‘fast_float::chars_format scn::v4::impl::{anonymous}::fast_float_impl_base::get_flags() const’:
/home/ben/src/forks/scnlib/src/scn/impl.cpp:726:26: error: invalid conversion from ‘int’ to ‘fast_float::chars_format’ [-fpermissive]
  726 |             format_flags |= fast_float::fixed;
      |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
      |                          |
      |                          int
/home/ben/src/forks/scnlib/src/scn/impl.cpp:729:26: error: invalid conversion from ‘int’ to ‘fast_float::chars_format’ [-fpermissive]
  729 |             format_flags |= fast_float::scientific;
      |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
      |                          |
      |                          int

Hmm.

[musicinmybrain]

The problem is that fast_float doesn't support long double. But the test is using long double:

BENCHMARK_TEMPLATE(scan_float_single_fastfloat, long double);

The error could be better though.

Hmm, that makes sense. (See fastfloat/fast_float#88.) What do you think should be done about it? And do you have any idea why it worked before fast_float 7.0.0?

[dalle]

The test compiled before because it used the !supported_float overload which in practice is an integer parser. Thank you, I'll add better errors for this.

[dalle]

Perhaps this then?

     fast_float::chars_format get_flags() const
     {
        fast_float::chars_format format_flags{};
         if ((m_options & float_reader_base::allow_fixed) != 0) {
            format_flags = static_cast<fast_float::chars_format>(format_flags | fast_float::chars_format::fixed);
         }
         if ((m_options & float_reader_base::allow_scientific) != 0) {
            format_flags = static_cast<fast_float::chars_format>(format_flags | fast_float::chars_format::scientific);
         }
         return format_flags;
     }

[musicinmybrain]

Perhaps this then?

     fast_float::chars_format get_flags() const
     {
        fast_float::chars_format format_flags{};
         if ((m_options & float_reader_base::allow_fixed) != 0) {
            format_flags = static_cast<fast_float::chars_format>(format_flags | fast_float::chars_format::fixed);
         }
         if ((m_options & float_reader_base::allow_scientific) != 0) {
            format_flags = static_cast<fast_float::chars_format>(format_flags | fast_float::chars_format::scientific);
         }
         return format_flags;
     }

That seems to work on both 6.1.6 and 7.0.0! I’m not enough of a C++ language lawyer to explain off the cuff why I’m allowed to bitwise-or two enum values using operator| but not operator|= – something to do with weird corner cases in implicit integer promotions, I’m sure – but if it works, it works.

This approach isn’t any simpler than what I originally proposed in #135 (comment), but it has the advantage that we don’t have to explicitly name the underlying type of the enum or enum class (or at least, some adequate integral type that we can use for or-ing flags together).

[dalle]

No. It is not any simpler or faster, the only thing it does is to mention the underlying type.

operator|(E, E) returns an int (or underlying type) for unscoped enums, but is undefined for scoped enums.

[musicinmybrain]

The test compiled before because it used the !supported_float overload which in practice is an integer parser. Thank you, I'll add better errors for this.

I see that your PR to improve the error message in fast_float was merged. Thanks!

What do you think we should do about this on the scnlib end? Considering fast_float doesn’t support long double, shall we just remove the lines that try to benchmark fast_float with long_double?

diff --git a/benchmark/runtime/float/repeated.cpp b/benchmark/runtime/float/repeated.cpp
index 0aa0c39..8a4de0a 100644
--- a/benchmark/runtime/float/repeated.cpp
+++ b/benchmark/runtime/float/repeated.cpp
@@ -210,4 +210,3 @@ static void scan_float_repeated_fastfloat(benchmark::State& state)
 }
 BENCHMARK_TEMPLATE(scan_float_repeated_fastfloat, float);
 BENCHMARK_TEMPLATE(scan_float_repeated_fastfloat, double);
-BENCHMARK_TEMPLATE(scan_float_repeated_fastfloat, long double);
diff --git a/benchmark/runtime/float/single.cpp b/benchmark/runtime/float/single.cpp
index e06cd13..6819621 100644
--- a/benchmark/runtime/float/single.cpp
+++ b/benchmark/runtime/float/single.cpp
@@ -185,4 +185,3 @@ static void scan_float_single_fastfloat(benchmark::State& state)
 }
 BENCHMARK_TEMPLATE(scan_float_single_fastfloat, float);
 BENCHMARK_TEMPLATE(scan_float_single_fastfloat, double);
-BENCHMARK_TEMPLATE(scan_float_single_fastfloat, long double);

[musicinmybrain]

What do you think we should do about this on the scnlib end? Considering fast_float doesn’t support long double, shall we just remove the lines that try to benchmark fast_float with long_double?

I went ahead and removed those two lines in #136, which should now actually compile and pass tests.