Fix 30 bugs found during code review

.github/workflows/windows.yaml

@@ -51,9 +51,9 @@ jobs:
           cmake -S . -B build -DCMAKE_CXX_STANDARD=20 -DREFLECTCPP_BUILD_TESTS=ON -DREFLECTCPP_JSON=OFF -DREFLECTCPP_${{ matrix.format }}=ON -DCMAKE_BUILD_TYPE=Release
           cmake --build build --config Release -j4 
       - name: Run tests
-        if: matrix.format != 'benchmarks' 
+        if: matrix.format != 'benchmarks'
         run: |
-          ctest --test-dir build --output-on-failure
+          ctest --test-dir build -C Release --output-on-failure
       - name: Run benchmarks 
         if: matrix.format == 'benchmarks' 
         run: |
@@ -106,6 +106,6 @@ jobs:
           cmake --build build --config Release -j4 
       - name: Run tests
         run: |
-          ctest --test-dir build --output-on-failure
+          ctest --test-dir build -C Release --output-on-failure
 
 

include/rfl/Field.hpp

@@ -34,7 +34,7 @@ struct Field {
   Field(const Field<_name, U>& _field) : value_(_field.get()) {}
 
   template <class U>
-  Field(Field<_name, U>&& _field) : value_(_field.get()) {}
+  Field(Field<_name, U>&& _field) : value_(std::move(_field.get())) {}
 
   template <class U>
     requires std::is_convertible_v<U, Type>

include/rfl/Flatten.hpp

@@ -29,15 +29,15 @@ struct Flatten {
   Flatten(const Flatten<U>& _f) : value_(_f.get()) {}
 
   template <class U>
-  Flatten(Flatten<U>&& _f) : value_(_f.get()) {}
+  Flatten(Flatten<U>&& _f) : value_(std::move(_f.get())) {}
 
   template <class U>
     requires std::is_convertible_v<U, Type>
   Flatten(const U& _value) : value_(_value) {}
 
   template <class U>
     requires std::is_convertible_v<U, Type>
-  Flatten(U&& _value) : value_(_value) {}
+  Flatten(U&& _value) : value_(std::forward<U>(_value)) {}
 
   ~Flatten() = default;
 
@@ -101,7 +101,7 @@ struct Flatten {
   /// Assigns the underlying object.
   template <class U>
   Flatten& operator=(Flatten<U>&& _f) {
-    value_ = std::forward<U>(_f);
+    value_ = std::move(_f.get());
     return *this;
   }
 

include/rfl/Generic.hpp

@@ -1,6 +1,7 @@
 #ifndef RFL_GENERIC_HPP_
 #define RFL_GENERIC_HPP_
 
+#include <limits>
 #include <optional>
 #include <ostream>
 #include <string>
@@ -177,6 +178,11 @@ class RFL_API Generic {
         [](auto _v) -> Result<int> {
           using V = std::remove_cvref_t<decltype(_v)>;
           if constexpr (std::is_same_v<V, int64_t>) {
+            if (_v < static_cast<int64_t>(std::numeric_limits<int>::min()) ||
+                _v > static_cast<int64_t>(std::numeric_limits<int>::max())) {
+              return error(
+                  "rfl::Generic: int64_t value out of range for int.");
+            }
             return static_cast<int>(_v);
           } else {
             return error(

include/rfl/Object.hpp

@@ -26,7 +26,7 @@ class Object {
   using mapped_type = T;
   using value_type = std::pair<std::string, T>;
   using size_type = typename DataType::size_type;
-  using difference_type = typename DataType::size_type;
+  using difference_type = typename DataType::difference_type;
   using reference = value_type&;
   using const_reference = const value_type&;
   using pointer = typename DataType::pointer;

include/rfl/Result.hpp

@@ -211,7 +211,10 @@ class Result {
     requires std::is_convertible_v<U, T>
   auto& operator=(const Result<U>& _other) {
     const auto to_t = [](const U& _u) -> T { return _u; };
-    t_or_err_ = _other.transform(to_t).t_or_err_;
+    auto temp = _other.transform(to_t);
+    destroy();
+    success_ = temp.success_;
+    move_from_other(temp);
     return *this;
   }
 
@@ -292,7 +295,7 @@ class Result {
   }
 
   /// Returns the value or a default.
-  T&& value_or(T&& _default) && noexcept {
+  T value_or(T&& _default) && noexcept {
     if (success_) {
       return std::move(*this).get_t();
     } else {
@@ -332,7 +335,7 @@ class Result {
 
   bool has_value() const noexcept { return success_; }
 
-  Error& error() && {
+  Error&& error() && {
     if (success_) throw std::runtime_error("Expected does not contain value");
     return std::move(*this).get_err();
   }

include/rfl/TaggedUnion.hpp

@@ -12,7 +12,7 @@ namespace rfl {
 // https://serde.rs/enum-representations.html
 template <internal::StringLiteral _discriminator, class... Ts>
 struct TaggedUnion {
-  static constexpr internal::StringLiteral discrimininator_ = _discriminator;
+  static constexpr internal::StringLiteral discriminator_ = _discriminator;
 
   /// The type of the underlying variant.
   using VariantType = rfl::Variant<Ts...>;

include/rfl/Tuple.hpp

@@ -102,17 +102,19 @@ class Tuple {
     static_assert(sizeof...(Types) == sizeof...(OtherTypes),
                   "The size of the two tuples must be the same.");
 
-    const auto compare = [&]<int _i>(std::strong_ordering* _ordering,
-                                     std::integral_constant<int, _i>) {
-      if (*_ordering != std::strong_ordering::equivalent &&
-          this->get<_i>() != _other.template get<_i>()) {
-        *_ordering = (this->get<_i>() <=> _other.template get<_i>());
-      }
+    using OrderingType = std::common_comparison_category_t<
+        decltype(std::declval<Types>() <=> std::declval<OtherTypes>())...>;
+
+    auto ordering = OrderingType::equivalent;
+
+    const auto compare = [&]<int _i>(std::integral_constant<int, _i>) {
+      ordering = static_cast<OrderingType>(
+          this->get<_i>() <=> _other.template get<_i>());
+      return ordering != 0;
     };
 
     return [&]<int... _is>(std::integer_sequence<int, _is...>) {
-      auto ordering = std::strong_ordering::equivalent;
-      (compare(&ordering, std::integral_constant<int, _is>{}), ...);
+      (compare(std::integral_constant<int, _is>{}) || ...);
       return ordering;
     }(std::make_integer_sequence<int, sizeof...(Types)>());
   }

include/rfl/bson/Reader.hpp

@@ -124,12 +124,24 @@ struct Reader {
       return value.v_bool;
 
     } else if constexpr (std::is_floating_point<std::remove_cvref_t<T>>()) {
-      if (btype != BSON_TYPE_DOUBLE) {
-        return error(
-            "Could not cast to numeric value. The type must be double, "
-            "int32, int64 or date_time.");
+      switch (btype) {
+        case BSON_TYPE_DOUBLE:
+          return static_cast<T>(value.v_double);
+
+        case BSON_TYPE_INT32:
+          return static_cast<T>(value.v_int32);
+
+        case BSON_TYPE_INT64:
+          return static_cast<T>(value.v_int64);
+
+        case BSON_TYPE_DATE_TIME:
+          return static_cast<T>(value.v_datetime);
+
+        default:
+          return error(
+              "Could not cast to numeric value. The type must be double, "
+              "int32, int64 or date_time.");
       }
-      return static_cast<T>(value.v_double);
 
     } else if constexpr (std::is_integral<std::remove_cvref_t<T>>()) {
       switch (btype) {

include/rfl/capnproto/read.hpp

@@ -78,7 +78,9 @@ auto read(std::istream& _stream) {
 
 template <class T, class... Ps>
 auto read(std::istream& _stream, const Schema<T>& _schema) {
-  return read<T, Ps...>(_stream, _schema);
+  std::istreambuf_iterator<char> begin(_stream), end;
+  auto bytes = std::vector<char>(begin, end);
+  return read<T, Ps...>(bytes.data(), bytes.size(), _schema);
 }
 
 }  // namespace rfl::capnproto

include/rfl/cli/Reader.hpp

@@ -1,7 +1,10 @@
 #ifndef RFL_CLI_READER_HPP_
 #define RFL_CLI_READER_HPP_
 
+#include <charconv>
 #include <concepts>
+#include <clocale>
+#include <cstdlib>
 #include <map>
 #include <optional>
 #include <set>
@@ -56,43 +59,43 @@ rfl::Result<T> parse_value(
       "Could not cast '" + _str + "' to boolean for key '" + _path + "'.");
 }
 
+// std::from_chars for float/double is unavailable in Apple libc++.
+// std::strtod depends on the C locale (LC_NUMERIC), so "3.14" can fail
+// under locales that use comma as decimal separator.
+// Use strtod_l with an explicit "C" locale on all platforms for consistency.
 template <class T> requires (std::is_floating_point_v<T>)
 rfl::Result<T> parse_value(
     const std::string& _str, const std::string& _path
 ) noexcept {
-  try {
-    return static_cast<T>(std::stod(_str));
-  }
-  catch (...) {
+  char* end = nullptr;
+#ifdef _WIN32
+  const auto c_locale = _create_locale(LC_NUMERIC, "C");
+  const double value = _strtod_l(_str.c_str(), &end, c_locale);
+  _free_locale(c_locale);
+#else
+  const auto c_locale = newlocale(LC_NUMERIC_MASK, "C", nullptr);
+  const double value = strtod_l(_str.c_str(), &end, c_locale);
+  freelocale(c_locale);
+#endif
+  if (end != _str.c_str() + _str.size()) {
     return error(
         "Could not cast '" + _str + "' to floating point for key '" + _path + "'.");
   }
+  return static_cast<T>(value);
 }
 
-template <class T> requires (std::is_unsigned_v<T> && !std::same_as<T, bool>)
+template <class T> requires (std::is_integral_v<T> && !std::same_as<T, bool>)
 rfl::Result<T> parse_value(
     const std::string& _str, const std::string& _path
 ) noexcept {
-  try {
-    return static_cast<T>(std::stoull(_str));
-  }
-  catch (...) {
-    return error(
-        "Could not cast '" + _str + "' to unsigned integer for key '" + _path + "'.");
-  }
-}
-
-template <class T> requires (std::is_integral_v<T> && std::is_signed_v<T>)
-rfl::Result<T> parse_value(
-    const std::string& _str, const std::string& _path
-) noexcept {
-  try {
-    return static_cast<T>(std::stoll(_str));
-  }
-  catch (...) {
+  T value;
+  const auto [ptr, ec] =
+      std::from_chars(_str.data(), _str.data() + _str.size(), value);
+  if (ec != std::errc() || ptr != _str.data() + _str.size()) {
     return error(
         "Could not cast '" + _str + "' to integer for key '" + _path + "'.");
   }
+  return value;
 }
 
 struct Reader {

include/rfl/internal/Skip.hpp

@@ -40,7 +40,7 @@ class Skip {
   Skip(const Skip<U, _skip_s, _skip_d>& _other) : value_(_other.get()) {}
 
   template <class U, bool _skip_s, bool _skip_d>
-  Skip(Skip<U, _skip_s, _skip_d>&& _other) : value_(_other.get()) {}
+  Skip(Skip<U, _skip_s, _skip_d>&& _other) : value_(std::move(_other.get())) {}
   template <class U>
     requires std::is_convertible_v<U, Type>
   Skip(const U& _value) : value_(_value) {}

include/rfl/internal/StringLiteral.hpp

@@ -14,14 +14,22 @@ namespace internal {
 /// for the parameters names in the NamedTuples.
 template <size_t N>
 struct StringLiteral {
-  constexpr StringLiteral(const auto... _chars) : arr_{_chars..., '\0'} {}
+  constexpr StringLiteral(const auto... _chars)
+    requires (std::is_same_v<decltype(_chars), const char> && ...)
+    : arr_{_chars..., '\0'} {}
 
   constexpr StringLiteral(const std::array<char, N> _arr) : arr_(_arr) {}
 
   constexpr StringLiteral(const char (&_str)[N]) {
     std::copy_n(_str, N, std::data(arr_));
   }
 
+  template <class T>
+    requires (std::is_same_v<T, const char*> || std::is_same_v<T, char*>)
+  explicit constexpr StringLiteral(T _data) {
+    std::copy_n(_data, N, std::data(arr_));
+  }
+
   /// Returns the value as a string.
   std::string str() const { return std::string(string_view()); }