Don't use std::visit for formatting. It is too slow and complex (#112)

* Don't use std::visit for formatting. It is too slow and complex Signed-off-by: John Sallay <[email protected]> * Try to make some compilers happy Signed-off-by: John Sallay <[email protected]> * Try to make some compilers happy Signed-off-by: John Sallay <[email protected]> * Try updating base image and compiler Signed-off-by: John Sallay <[email protected]> * Install meson and ninja in new container Signed-off-by: John Sallay <[email protected]> * Add sudo on installs Signed-off-by: John Sallay <[email protected]> * Need one more package Signed-off-by: John Sallay <[email protected]> * Need to add sudo Signed-off-by: John Sallay <[email protected]> * Use new gnuradio images Signed-off-by: John Sallay <[email protected]> * Needed to change name in one more place Signed-off-by: John Sallay <[email protected]> * Need to use std::visit to make compiler happy Signed-off-by: John Sallay <[email protected]> * Update fmt dep so that we can compile everywhere Signed-off-by: John Sallay <[email protected]> * Install clang in emscripten jobs Signed-off-by: John Sallay <[email protected]> * Only require c++20. Signed-off-by: John Sallay <[email protected]> --------- Signed-off-by: John Sallay <[email protected]>
gnuradio · Oct 2, 2024 · 6d08b2c · 6d08b2c
1 parent 2949e0e
commit 6d08b2c
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 65 deletions.
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
@@ -9,7 +9,7 @@ on:
 jobs:
   linux-docker:
   # All of these shall depend on the formatting check (needs: check-formatting)
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-24.04
     # The GH default is 360 minutes (it's also the max as of Feb-2021). However,
     # we should fail sooner. The only reason to exceed this time is if a test
     # hangs.
@@ -23,11 +23,11 @@ jobs:
         # descriptive name, and one key 'containerid' with the name of the
         # container (i.e., what you want to docker-pull)
         distro:
-          - name: 'Ubuntu 22.04'
-            containerid: 'ghcr.io/gnuradio/gnuradio-docker:ubuntu-22.04'
+          - name: 'Ubuntu 24.04'
+            containerid: 'ghcr.io/gnuradio/gnuradio-docker:ubuntu-24.04'
             cxxflags: -Werror
-          - name: 'Fedora 36'
-            containerid: 'ghcr.io/gnuradio/gnuradio-docker:fedora-36'
+          - name: 'Fedora 40'
+            containerid: 'ghcr.io/gnuradio/gnuradio-docker:fedora-40'
             cxxflags: ''
           # - distro: 'CentOS 8.3'
           #   containerid: 'gnuradio/ci:centos-8.3-3.9'

diff --git a/.github/workflows/emscripten.yml b/.github/workflows/emscripten.yml
@@ -9,7 +9,7 @@ on:
 jobs:
   linux-docker:
   # All of these shall depend on the formatting check (needs: check-formatting)
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-24.04
     # The GH default is 360 minutes (it's also the max as of Feb-2021). However,
     # we should fail sooner. The only reason to exceed this time is if a test
     # hangs.
@@ -23,8 +23,8 @@ jobs:
         # descriptive name, and one key 'containerid' with the name of the
         # container (i.e., what you want to docker-pull)
         distro:
-          - name: 'Ubuntu 22.04'
-            containerid: 'ghcr.io/gnuradio/gnuradio-docker:ubuntu-22.04'
+          - name: 'Ubuntu 24.04'
+            containerid: 'ghcr.io/gnuradio/gnuradio-docker:ubuntu-24.04'
             cxxflags: -Werror
         compiler:
           - name: "emscripten"
@@ -40,7 +40,7 @@ jobs:
       name: Checkout Project
     - name: Install emscripten
       run: |
-        DEBIAN_FRONTEND=noninteractive apt-get install -qy bzip2
+        DEBIAN_FRONTEND=noninteractive apt-get install -qy bzip2 clang
         cd
         git clone https://github.com/emscripten-core/emsdk.git
         cd emsdk

diff --git a/include/pmtv/format.hpp b/include/pmtv/format.hpp
@@ -2,6 +2,7 @@
 // Gcc12 does not support it.
 // Eventually replace with std::format when that is widely available.
 #include <fmt/format.h>
+#include <fmt/ranges.h>
 
 namespace fmt {
 template <>
@@ -45,25 +46,7 @@ struct formatter<P>
 
     template <typename FormatContext>
     auto format(const P& value, FormatContext& ctx) const {
-        // Due to an issue with the c++ spec that has since been resolved, we have to do something
-        // funky here.  See
-        // https://stackoverflow.com/questions/37526366/nested-constexpr-function-calls-before-definition-in-a-constant-expression-con
-        // This problem only appears to occur in gcc 11 in certain optimization modes. The problem
-        // occurs when we want to format a vector<pmt>.  Ideally, we can write something like:
-        //      return fmt::format_to(ctx.out(), "[{}]", fmt::join(arg, ", "));
-        // It looks like the issue effects clang 14/15 as well.
-        // However, due to the above issue, it fails to compile.  So we have to do the equivalent
-        // ourselves.  We can't recursively call the formatter, but we can recursively call a lambda
-        // function that does the formatting.
-        // It gets more complicated, because we need to pass the function into the lambda.  We can't
-        // pass in the lamdba as it is defined, so we create a nested lambda.  Which accepts a function
-        // as a argument.
-        // Because we are calling std::visit, we can't pass non-variant arguments to the visitor, so we
-        // have to create a new nested lambda every time we format a vector to ensure that it works.
-        using namespace pmtv;
-        using ret_type = decltype(fmt::format_to(ctx.out(), ""));
-        auto format_func = [&ctx](const auto format_arg) {
-            auto function_main = [&ctx](const auto arg, auto function) -> ret_type {
+        return std::visit([&ctx](const auto arg) {
             using namespace pmtv;
             using T = std::decay_t<decltype(arg)>;
             if constexpr (Scalar<T> || Complex<T>)
@@ -73,38 +56,13 @@ struct formatter<P>
             else if constexpr (UniformVector<T> || UniformStringVector<T>)
                 return fmt::format_to(ctx.out(), "[{}]", fmt::join(arg, ", "));
             else if constexpr (std::same_as<T, std::vector<pmt>>) {
-                fmt::format_to(ctx.out(), "[");
-                auto new_func = [&function](const auto new_arg) -> ret_type { return function(new_arg, function); };
-                for (auto& a: std::span(arg).first(arg.size()-1)) {
-                    std::visit(new_func, a);
-                    fmt::format_to(ctx.out(), ", ");
-                }
-                std::visit(new_func, arg[arg.size()-1]);
-                return fmt::format_to(ctx.out(), "]");
-                // When we drop support for gcc11/clang15, get rid of the nested lambda and replace
-                // the above with this line.
-                //return fmt::format_to(ctx.out(), "[{}]", fmt::join(arg, ", "));
+                return fmt::format_to(ctx.out(), "[{}]", fmt::join(arg, ", "));
             } else if constexpr (PmtMap<T>) {
-                fmt::format_to(ctx.out(), "{{");
-                auto new_func = [&function](const auto new_arg) -> ret_type { return function(new_arg, function); };
-                size_t i = 0;
-                for (auto& [k, v]: arg) {
-                    fmt::format_to(ctx.out(), "{}: ", k);
-                    std::visit(new_func, v);
-                    if (i++ < arg.size() - 1)
-                        fmt::format_to(ctx.out(), ", ");
-                }
-                return fmt::format_to(ctx.out(), "}}");
-                // When we drop support for gcc11/clang15, get rid of the nested lambda and replace
-                // the above with this line.
-                //return fmt::format_to(ctx.out(), "{{{}}}", fmt::join(arg, ", "));
+                return fmt::format_to(ctx.out(), "{{{}}}", fmt::join(arg, ", "));
             } else if constexpr (std::same_as<std::monostate, T>)
                 return fmt::format_to(ctx.out(), "null");
             return fmt::format_to(ctx.out(), "unknown type {}", typeid(T).name());
-            };
-            return function_main(format_arg, function_main);
-        };
-        return std::visit(format_func, value);
+            }, value);
 
     }
 };

diff --git a/meson.build b/meson.build
@@ -98,7 +98,7 @@ endif
 gtest_dep = dependency('gtest', main : true, version : '>=1.10', required : get_option('enable_testing'))
 gtest_main_dep = dependency('gtest_main', version : '>=1.10', required : get_option('enable_testing'))
 CLI11_dep = dependency('CLI11', fallback : [ 'cli11' , 'CLI11_dep' ])
-fmt_dep = dependency('fmt', version:'>=8.1.1')
+fmt_dep = dependency('fmt', version:'>=10.0.0')
 
 cmake = import('cmake')
 

diff --git a/subprojects/fmt.wrap b/subprojects/fmt.wrap
@@ -1,12 +1,12 @@
 [wrap-file]
-directory = fmt-8.1.1
-source_url = https://github.com/fmtlib/fmt/archive/8.1.1.tar.gz
-source_filename = fmt-8.1.1.tar.gz
-source_hash = 3d794d3cf67633b34b2771eb9f073bde87e846e0d395d254df7b211ef1ec7346
-patch_filename = fmt_8.1.1-2_patch.zip
-patch_url = https://wrapdb.mesonbuild.com/v2/fmt_8.1.1-2/get_patch
-patch_hash = cd001046281330a8862591780a9ea71a1fa594edd0d015deb24e44680c9ea33b
-wrapdb_version = 8.1.1-2
+directory = fmt-11.0.2
+source_url = https://github.com/fmtlib/fmt/archive/11.0.2.tar.gz
+source_filename = fmt-11.0.2.tar.gz
+source_hash = 6cb1e6d37bdcb756dbbe59be438790db409cdb4868c66e888d5df9f13f7c027f
+patch_filename = fmt_11.0.2-1_patch.zip
+patch_url = https://wrapdb.mesonbuild.com/v2/fmt_11.0.2-1/get_patch
+patch_hash = 90c9e3b8e8f29713d40ca949f6f93ad115d78d7fb921064112bc6179e6427c5e
+wrapdb_version = 11.0.2-1
 
 [provide]
 fmt = fmt_dep