Merge pull request #98 from Flow-IPC/open_tkt95_cxx20_support

C++20 mode build and Github-CI-pipline support. C++17 remains the main/minimal mode, but we now ensure user can build/use in C++20 mode also.

Author: ygoldfeld

.github/workflows/main.yml

@@ -139,7 +139,44 @@ jobs:
 
     strategy:
       fail-fast: false
+
+      # Our basic matrix is compiler-version x build-test-config.  The latter is the usual possibilities namely
+      # Debug, Release, ..., plus a few runtime-sanitizer-oriented variations.  Generally we want to run the entire
+      # cross-product AxB of possibilities; but for various reasons we exclude some specific combinations from the
+      # matrix. So far, then: specify full matrix, specify certain combos to remove from it.
+      #
+      # There is however another variable: the C++ standard to specify.  In actual practice -- as explained in
+      # FlowLikeCodeGenerate.cmake, but we *very* briefly recap -- the min required standard is 17, and it is indeed
+      # *the* standard we use, never taking advantage (via conditional compilation, etc.) of 20-or-higher.  So,
+      # the main matrix shall built with std=17.  In addition, though, we *allow* C++20 or higher.  In practice, 99.999%
+      # C++17 code shall build equivalently with C++20 (as for C++23, we haven't looked into it as of this writing);
+      # but not 100%, and indeed for business reasons we found it is worthwhile to *test* with C++20 as well, so
+      # as to ensure code will (at least) build (and thus not mess-over C++20 users; it is now 2025 as of this writing).
+      # However, loosely speaking, we need only hit a few basic setups likeliest to trigger problems (usually warnings)
+      # which we'd then fix in the code.  Long story short then: in addition to the aforementioned matrix (w/ 17),
+      # minus exclusions, we would *add* a few specific configs with std=20.
+      #
+      # Delightfully that's just how GitHub Actions matrix specs work: The matrix, then remove `exclude`s, then
+      # add `include`s.  ...Unfortunately, nope, the `include`s cannot simply refer to earlier-defined things in
+      # the matrix, so for example all the properties of a given compiler (e.g. gcc-13: name, version, c-path, cpp-path,
+      # install) would need to be copy/pasted (stylistically awful/error-prone).  (TODO: Perhaps there's some way
+      # to predefine each compiler's "stuff" in some earlier variable type thing and then just refer to it -- maybe
+      # something with outputs and fromJSON()... I (ygoldfel) was unable to find clean solid docs/example(s) thereof,
+      # but a real Actions+YAML badass could probably figure it out.  Maybe.  OH!  And YAML anchors/aliases would be
+      # *perfect* for this... but Actions guys have somehow not implemented them; it is a long-standing
+      # request [https://github.com/actions/runner/issues/1182#issuecomment-2317953582].)  Hence, for now, we are
+      # forced to express these post-exclude-includes as simply more excludes.  E.g., instead of saying "for C++20
+      # do just gcc-13" say "do the full matrix but exclude C++20 + gcc-9,10,11."
+      #
+      # So now do all that.
       matrix:
+        # First axis in basic 3D matrix is the 2 possible C++ standards.
+        # For simplicity the Conan-profile-configuring script step simply keys off the `id` instead of our defining
+        # other properties as for the other 2 more complex axes below (compiler, build config).
+        cxx-std:
+          - id: cxx17
+          - id: cxx20
+        # Second axis = the many compilers (gcc multiple versions, clang multiple versions).
         compiler:
           - id: gcc-9
             name: gcc
@@ -184,6 +221,7 @@ jobs:
             c-path: /usr/bin/clang-17
             cpp-path: /usr/bin/clang++-17
             install: True
+        # Last axis are the many build/test configs.
         build-test-cfg:
           - id: debug
             conan-profile-build-type: Debug
@@ -245,43 +283,111 @@ jobs:
             sanitizer-name: tsan
             no-lto: True
 
-        # Again, these exclusions are explained in FLow-IPC workflow counterpart.
+        # Remove certain values from the 3D matrix so far.
+        # The overall strategy is outlined in a large-ish comment above.
         exclude:
-          - compiler:  { id: gcc-9 }
+          # Firstly the exclusions from the C++17 sub-matrix (the other being the C++20 sub-matrix).
+          # Again, these exclusions are explained in Flow-IPC workflow counterpart.
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-9 }
             build-test-cfg: { id: relwithdebinfo-asan }
-          - compiler:  { id: gcc-10 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-10 }
             build-test-cfg: { id: relwithdebinfo-asan }
-          - compiler:  { id: gcc-11 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-11 }
             build-test-cfg: { id: relwithdebinfo-asan }
-          - compiler:  { id: gcc-13 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-13 }
             build-test-cfg: { id: relwithdebinfo-asan }
-          - compiler:  { id: clang-13 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: clang-13 }
             build-test-cfg: { id: relwithdebinfo-asan }
-          - compiler:  { id: gcc-9 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-9 }
             build-test-cfg: { id: relwithdebinfo-ubsan }
-          - compiler:  { id: gcc-10 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-10 }
             build-test-cfg: { id: relwithdebinfo-ubsan }
-          - compiler:  { id: gcc-11 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-11 }
             build-test-cfg: { id: relwithdebinfo-ubsan }
-          - compiler:  { id: gcc-13 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-13 }
             build-test-cfg: { id: relwithdebinfo-ubsan }
-          - compiler:  { id: clang-13 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: clang-13 }
             build-test-cfg: { id: relwithdebinfo-ubsan }
-          - compiler:  { id: gcc-9 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-9 }
+            build-test-cfg: { id: relwithdebinfo-tsan }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-10 }
             build-test-cfg: { id: relwithdebinfo-tsan }
-          - compiler:  { id: gcc-10 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-11 }
             build-test-cfg: { id: relwithdebinfo-tsan }
-          - compiler:  { id: gcc-11 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: gcc-13 }
             build-test-cfg: { id: relwithdebinfo-tsan }
-          - compiler:  { id: gcc-13 }
+          - cxx-std: { id: cxx17 }
+            compiler: { id: clang-13 }
             build-test-cfg: { id: relwithdebinfo-tsan }
-          - compiler:  { id: clang-13 }
+          # Now the exclusions from the C++20 sub-matrix.
+          # The basic strategy is noted above; but specifically release-oriented builds tend to go down different
+          # paths/produce different warnings vs debug-oriented builds -- so we want to try both; and as for which
+          # compilers to use, on one hand we'll definitely want at least 1 of gcc and clang each, but beyond that
+          # just the highest version of each is sufficient.  TODO: Revisit, especially if users report C++20 problems
+          # in other environments, and therefore we will have had to fix them; hence test coverage should increase.
+          #
+          # So, we essentially want something like:
+          #   include:
+          #   # gcc-<highest> x (2 build-configs).
+          #   - cxx-std: { id: cxx20 }
+          #     compiler: { id: gcc-13 }
+          #     build-test-cfg: { id: debug }
+          #   - cxx-std: { id: cxx20 }
+          #     compiler: { id: gcc-13 }
+          #     build-test-cfg: { id: relwithdebinfo }
+          #   # clang-<highest> x (2 build-configs).
+          #   - cxx-std: { id: cxx20 }
+          #     compiler: { id: clang-17 }
+          #     build-test-cfg: { id: debug }
+          #   - cxx-std: { id: cxx20 }
+          #     compiler: { id: clang-17 }
+          #     build-test-cfg: { id: relwithdebinfo }
+          # Sadly, as mentioned in the large-ish comment nearer the top, `include` does not let us refer to
+          # earlier-defined things simply by their `id`s.  So we choose to express the inclusions as exclusions
+          # instead:
+          #   - First, simply exclude all gccs but gcc-13 and all clangs but clang-17.
+          - cxx-std: { id: cxx20 }
+            compiler: { id: gcc-9 }
+          - cxx-std: { id: cxx20 }
+            compiler: { id: gcc-10 }
+          - cxx-std: { id: cxx20 }
+            compiler: { id: gcc-11 }
+          - cxx-std: { id: cxx20 }
+            compiler: { id: clang-13 }
+          - cxx-std: { id: cxx20 }
+            compiler: { id: clang-15 }
+          - cxx-std: { id: cxx20 }
+            compiler: { id: clang-16 }
+          #   - Second, among the remaining C++20 configs eliminate all but debug and relwithdebinfo ones.
+          - cxx-std: { id: cxx20 }
+            build-test-cfg: { id: release }
+          - cxx-std: { id: cxx20 }
+            build-test-cfg: { id: minsizerel }
+          - cxx-std: { id: cxx20 }
+            build-test-cfg: { id: relwithdebinfo-asan }
+          - cxx-std: { id: cxx20 }
+            build-test-cfg: { id: relwithdebinfo-ubsan }
+          - cxx-std: { id: cxx20 }
             build-test-cfg: { id: relwithdebinfo-tsan }
 
     # Not using ubuntu-latest, so as to avoid surprises with OS upgrades and such.
     runs-on: ubuntu-22.04
 
-    name: ${{ matrix.compiler.id }}-${{ matrix.build-test-cfg.id }}
+    name: ${{ matrix.compiler.id }}-${{ matrix.build-test-cfg.id }}-${{ matrix.cxx-std.id }}
 
     env:
       build-dir: ${{ github.workspace }}/build/${{ matrix.build-test-cfg.conan-profile-build-type }}
@@ -383,7 +489,7 @@ jobs:
         [settings]
         compiler = ${{ matrix.compiler.name }}
         compiler.version = ${{ matrix.compiler.version }}
-        compiler.cppstd = 17
+        compiler.cppstd = ${{ ((matrix.cxx-std.id == 'cxx20') && '20') || '17' }}
         # TODO: Consider testing with LLVM-libc++ also (with clang anyway).
         compiler.libcxx = libstdc++11
         arch = x86_64
@@ -403,6 +509,10 @@ jobs:
         [options]
         flow:build = True
         flow:doc = False
+        # `0` here as of this writing would effectively mean `17` too.  Specifying `17` explicitly has the same
+        # effect in the end but goes down a slightly different code-path in FlowLikeCodeGenerate.cmake.  It's not
+        # a huge deal... but it's a little nice to cover more paths, so let's do the `0` path when applicable.
+        flow:build_cxx_std = ${{ ((matrix.cxx-std.id == 'cxx20') && '20') || '0' }}
         EOF
 
     - name: Install Flow dependencies with Conan using the profile
@@ -517,7 +627,7 @@ jobs:
         always()
       uses: actions/upload-artifact@v4
       with:
-        name: flow-test-logs-${{ matrix.compiler.id }}-${{ matrix.build-test-cfg.id }}
+        name: flow-test-logs-${{ matrix.compiler.id }}-${{ matrix.build-test-cfg.id }}-${{ matrix.cxx-std.id }}
         path: ${{ env.install-dir }}/bin/logs.tgz
 
   doc-and-release:

conanfile.py

@@ -35,12 +35,18 @@ class FlowRecipe(ConanFile):
     options = {
         "build": [True, False],
         "build_no_lto": [True, False],
+
+        # 0 => default (let build script decide, as of this writing 17 meaning C++17) or a #, probably `20` as of
+        # this writing.
+        "build_cxx_std": ["ANY"],
+
         "doc": [True, False]
     }
 
     default_options = {
         "build": True,
         "build_no_lto": False,
+        "build_cxx_std": 0,
         "doc": False
     }
 
@@ -83,6 +89,8 @@ def generate(self):
         if self.options.build:
             if self.options.build_no_lto:
                 toolchain.variables["CFG_NO_LTO"] = "ON"
+            if self.options.build_cxx_std != 0:
+                toolchain.variables["CMAKE_CXX_STANDARD"] = self.options.build_cxx_std
         else:
             toolchain.variables["CFG_SKIP_CODE_GEN"] = "ON"
         if self.options.doc:

src/flow/common.hpp

@@ -70,7 +70,12 @@
  *
  * Note this isn't academic; as of this writing there's at least a C++14-requiring constexpr feature in use in one
  * of the headers.  So at least C++14 has been required for ages in actual practice.  Later, notched it up to C++17
- * by similar logic. */
+ * by similar logic.
+ *
+ * Update: We continue to target C++17 (by default) in our build -- but now also support (and, outside
+ * the source code proper, test) C++20 mode build, albeit without using any C++20-only language or STL features.  It is
+ * conceivable that at some point in the future we'll target C++20 by default (and drop support for C++17 and older).
+ * Naturally at that point we'd begin using C++20 language/STL features when convenient. */
 #if (!defined(__cplusplus)) || (__cplusplus < 201703L)
 // Would use static_assert(false), but... it's C++11 and later only.  So.
 #  error "To compile a translation unit that `#include`s any flow/ API headers, use C++17 compile mode or later."

src/flow/net_flow/detail/low_lvl_packet.hpp

@@ -27,6 +27,7 @@
 #include "flow/util/blob.hpp"
 #include <boost/endian.hpp>
 #include <limits>
+#include <type_traits>
 
 namespace flow::net_flow
 {
@@ -1220,19 +1221,15 @@ struct Ack_packet::Individual_ack
    */
   const unsigned int m_rexmit_id;
 
-  // Constructors/destructor.
-
-  /// Force direct member initialization even if no member is `const`.
-  Individual_ack() = delete;
-
-  /// Forbid copy construction.
-  Individual_ack(const Individual_ack&) = delete;
+  /// Make us noncopyable without breaking aggregateness (direct-init).
+  [[no_unique_address]] util::Noncopyable m_nc{};
+}; // struct Ack_packet::Individual_ack
 
-  // Methods.
-
-  /// Forbid copy assignment.
-  void operator=(const Individual_ack&) = delete;
-};
+static_assert(std::is_aggregate_v<Ack_packet::Individual_ack>,
+              "We want it to be direct-initializable.");
+static_assert((!std::is_copy_constructible_v<Ack_packet::Individual_ack>)
+                && (!std::is_copy_assignable_v<Ack_packet::Individual_ack>),
+              "We want it to be noncopyable but rather passed-around via its ::Ptr.");
 
 #pragma pack(push, 1)
 

src/flow/net_flow/peer_socket.cpp

@@ -1591,6 +1591,12 @@ void Node::async_acknowledge_packet(Peer_socket::Ptr sock, const Sequence_number
 
   const size_t acks_pending_before_this = sock->m_rcv_pending_acks.size();
 
+  static_assert(std::is_aggregate_v<Peer_socket::Individual_ack>,
+                "We want it to be direct-initializable.");
+  static_assert((!std::is_copy_constructible_v<Peer_socket::Individual_ack>)
+                  && (!std::is_copy_assignable_v<Peer_socket::Individual_ack>),
+                "We want it to be noncopyable but rather passed-around via its ::Ptr.");
+
   /* Just the starting sequence number sufficient to identify a single packet.  The time point saved
    * here is subtracted from time_now() at ACK send time, to compute the artificial delay introduced
    * by ACK delaying (explained just below).  This helps other side calculate a more accurate RTT by

src/flow/net_flow/peer_socket.hpp

@@ -36,6 +36,7 @@
 #include <boost/shared_ptr.hpp>
 #include <boost/enable_shared_from_this.hpp>
 #include <boost/move/unique_ptr.hpp>
+#include <type_traits>
 
 namespace flow::net_flow
 {
@@ -2408,16 +2409,10 @@ struct Peer_socket::Individual_ack
   /// Number of bytes in the packet's user data.
   const size_t m_data_size;
 
-  // Constructors/destructor.
-
-  /// Force direct member initialization even if no member is `const`.
-  Individual_ack(const Individual_ack&) = delete;
-
-  // Methods.
-
-  /// Forbid copy assignment.
-  Individual_ack& operator=(const Individual_ack&) = delete;
+  /// Make us noncopyable without breaking aggregateness (direct-init).
+  [[no_unique_address]] util::Noncopyable m_nc{};
 }; // struct Peer_socket::Individual_ack
+// Note: Some static_assert()s about it currently in peer_socket.cpp in a function {} (for boring reasons).
 
 // Free functions: in *_fwd.hpp.
 

src/flow/util/util.hpp

@@ -60,6 +60,31 @@ class Null_interface
   virtual ~Null_interface() = 0;
 };
 
+/**
+ * Useful as a no-unique-address private member to make a type noncopyable while keeping that type an aggregate
+ * (can be direct-initialized).
+ *
+ * So you can do: `[[no_unique_address]] flow::util::Noncopyable m_nc{};`.
+ *
+ * ### Rationale ###
+ * The usual technique of deriving from `boost::noncopyable` disables aggregateness.  In C++20 declaring
+ * a `= delete` copy ctor also disables it.  This trick still works though.
+ */
+struct Noncopyable
+{
+  // Constructors/destructor.
+
+  /// Makes it possible to instantiate.
+  Noncopyable() = default;
+  /// Forbid copying.
+  Noncopyable(const Noncopyable&) = delete;
+
+  // Methods.
+
+  /// Forbid copying.
+  void operator=(const Noncopyable&) = delete;
+};
+
 /**
  * A simple RAII-pattern class template that, at construction, sets the specified location in memory to a specified
  * value, memorizing the previous contents; and at destruction restores the value.  E.g.:

tools/cmake/FlowLikeCodeGenerate.cmake

@@ -267,13 +267,23 @@ message(VERBOSE "Environment checks passed.  Configuring build environment.")
 
 # Set CMake global stuff.
 
-# Compile our own source files in C++17 mode; and refuse to proceed, if the compiler does not support it.
-# Note that any `#include`r will also need to follow this, as our headers are not shy about using C++17 features;
-# but that's enforced by a check in common.hpp (at least); not by us here somehow.
-set(CXX_STD 17)
-set(CMAKE_CXX_STANDARD ${CXX_STD})
+# CMAKE_CXX_STANDARD controls the C++ standard version with which items are compiled.
+# We require C++17 at the lowest for compilation (and for any `#include`r -- but that is enforced via compile-time
+# check in universally-included common.hpp, not by us here somehow).
+# So if CMAKE_CXX_STANDARD is not specified by CMake invoker, we in fact build in C++17 mode.
+# If it *is* specified, then we don't override it. In practice, as of this writing, that means it should be 17
+# or 20. (If a lower one is attempted, we don't fight it here -- but common.hpp check will defeat it anyway.)
+
+# Won't decay to lower standard if compiler does not support CMAKE_CXX_STANDARD (will fail instead).
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
-message(STATUS "C++${CXX_STD} language and STL required.")
+
+if(NOT DEFINED CMAKE_CXX_STANDARD)
+  set(CMAKE_CXX_STANDARD 17)
+  message(STATUS "C++${CMAKE_CXX_STANDARD} language and STL required: set by this build script.")
+else()
+  message(STATUS "C++${CMAKE_CXX_STANDARD} language and STL requirement "
+                   "inherited from externally set CMAKE_CXX_STANDARD.")
+endif()
 
 # When we do find_package(Threads) to link threading library this will cause it to
 # prefer -pthread flag where applicable (as of this writing, just Linux, but perhaps any *nix ultimately).