[clockblocker]

rfl::CopyableBox is documented as copyable, but only its copy constructor works. box2 = box1 does not compile, and neither does copy-assigning a struct that holds a CopyableBox directly, such as DecisionTree::Node from the docs and tests.

Summary

Two commits. The first adds a test that does not build on main. The second is the fix:

 template <class T, Copyability C = Copyability::NON_COPYABLE>
 class Box {
-  Box& operator=(const Box<T>& other) requires (C == Copyability::COPYABLE) {
+  Box& operator=(const Box& other) requires (C == Copyability::COPYABLE) {
     if(this != &other) {
       ptr_ = std::make_unique<T>(*other);
     }
     return *this;
   }

   Box& operator=(const Box& _other) requires (C == Copyability::NON_COPYABLE) = delete;

Cause: inside the class, Box<T> means Box<T, NON_COPYABLE>. For a CopyableBox, the operator was therefore an assignment from the other Box type, not a copy assignment. The deleted operator=(const Box&) is a user-declared copy assignment, so the compiler adds no implicit one either. A CopyableBox had no copy assignment at all.

The body is unchanged. It copy-constructs the new T before the old one is destroyed, so assigning from a Box inside the value being replaced is safe (root = root->...lesser, covered by the test).

Evidence

• Before (main): the test from the first commit does not compile:

  test_box.cpp:103:8: error: no viable overloaded '='
      box2 = box1;
  test_box.cpp:115:9: error: object of type 'DecisionTree::Node' cannot be assigned because its copy assignment operator is implicitly deleted
      node2 = node1;
  test_box.cpp:125:8: error: no viable overloaded '='
      root = rfl::get<DecisionTree::Node>(root->leaf_or_node.get().variant()).lesser;
  

  After: generic.copyable_box_copy_assignment passes. All local suites pass: generic 99, json 265, cli 71, env 41, json_c_arrays_and_inheritance 7. test_box.cpp also passes -Wall -Werror syntax checks with Apple clang 17 (-std=c++20) and GCC 16.2 (-std=c++20, and -std=c++23 -DREFLECTCPP_USE_STD_EXPECTED). Apple clang with -std=c++23 fails on the two older tests in test_box.cpp (Box() is instantiated for a DecisionTree that has no default constructor). The errors are the same on main, so this PR does not cause them.

• AddressSanitizer (Clang and GCC 16), assigning from a Box inside the value it replaces:

  • root = root->...lesser with the test's DecisionTree, and with a depth-2 subtree of a depth-4 tree: clean with this PR. It is also clean with an in-place *ptr_ = *other, because rfl::Variant::operator= copies into a temporary before it destroys the current alternative.
  • root = root->children[0] with struct Tree { double value; std::vector<rfl::CopyableBox<Tree>> children; }: clean with this PR. In-place gives a heap-use-after-free with GCC/libstdc++ and a container-overflow with Clang/libc++.

• Benchmark on test_copyable_box::DecisionTree, Apple M1, -std=c++20 -O3 -DNDEBUG, Apple clang 17 and GCC 16.2. Each value is the best of 8 interleaved runs, and each run takes the best of 15 samples. Allocations were counted by replacing operator new.

  • before (e0ee3f6): b = a does not compile, so this measures the only workaround, b = rfl::CopyableBox<DecisionTree>(a) and n2 = DecisionTree::Node(n1).
  • after (this PR): b = a and n2 = n1.
  • in-place: this PR with *ptr_ = *other, measured for reference.

  Clang on the first line, GCC on the second.

  Assignment	before (workaround)	after (this PR)	in-place (rejected)
  box, one leaf	19.4 ns / 18.5 ns	19.2 ns / 18.5 ns	1.9 ns / 1.0 ns
  box, depth-10 tree (2047 nodes)	93.4 µs / 93.2 µs	94.6 µs / 92.3 µs	94.2 µs / 92.3 µs
  node with two leaves	61.0 ns / 58.2 ns	38.9 ns / 37.0 ns	3.7 ns / 2.2 ns
  node with two depth-9 subtrees (2047 nodes)	93.1 µs / 93.0 µs	93.6 µs / 92.3 µs	93.6 µs / 92.0 µs

  Heap allocations per assignment, Clang on the first line, GCC on the second:

  Assignment	before (workaround)	after (this PR)	in-place (rejected)
  box, one leaf	1 alloc / 1 alloc	1 alloc / 1 alloc	0 allocs / 0 allocs
  box, depth-10 tree (2047 nodes)	2047 allocs / 2047 allocs	2047 allocs / 2047 allocs	2046 allocs / 2046 allocs
  node with two leaves	2 allocs / 2 allocs	2 allocs / 2 allocs	0 allocs / 0 allocs
  node with two depth-9 subtrees (2047 nodes)	2046 allocs / 2046 allocs	2046 allocs / 2046 allocs	2044 allocs / 2044 allocs

  The fix adds no cost. For boxes, b = a matches the workaround, with the trees within ±1.2%. For a node with two leaves, n2 = n1 is about 22 ns faster than the workaround, with the same allocations; the workaround also builds and destroys a temporary Node. In-place saves the allocation, which is visible on small boxes. It is not used here because, with in-place, assigning from a Box inside the value being replaced is only safe if T's own copy assignment allows it (see the std::vector case above). The docs say CopyableBox calls the contained type's copy assignment operator. The existing body copy-constructs instead, and this PR keeps that.

Benchmark source (bench.cpp)

Built once per header tree (git archive <sha> include), with -DWORKAROUND for e0ee3f6.

// Copy assignment of rfl::CopyableBox<DecisionTree> and of a struct holding it.
// DecisionTree is test_copyable_box::DecisionTree from tests/generic/test_box.cpp.
// Build with -DWORKAROUND for the "before" tree (e0ee3f6), where `b = a`
// does not compile and the only option is assigning from an explicit copy.
#include <algorithm>
#include <chrono>
#include <cstdio>
#include <cstdlib>
#include <new>
#include <rfl.hpp>

static std::size_t g_allocs = 0;

void* operator new(std::size_t n) {
  ++g_allocs;
  if (void* p = std::malloc(n ? n : 1)) return p;
  throw std::bad_alloc();
}
void operator delete(void* p) noexcept { std::free(p); }
void operator delete(void* p, std::size_t) noexcept { std::free(p); }

struct DecisionTree {
  struct Leaf {
    using Tag = rfl::Literal<"Leaf">;
    double value;
  };

  struct Node {
    using Tag = rfl::Literal<"Node">;
    rfl::Rename<"criticalValue", double> critical_value;
    rfl::CopyableBox<DecisionTree> lesser;
    rfl::CopyableBox<DecisionTree> greater;
  };

  using LeafOrNode = rfl::TaggedUnion<"type", Leaf, Node>;

  rfl::Field<"leafOrNode", LeafOrNode> leaf_or_node;
};

using Box = rfl::CopyableBox<DecisionTree>;

static Box make_tree(int depth, double& next) {
  if (depth == 0) {
    return rfl::make_copyable_box<DecisionTree>(
        DecisionTree{DecisionTree::Leaf{.value = next++}});
  }
  auto l = make_tree(depth - 1, next);
  auto g = make_tree(depth - 1, next);
  return rfl::make_copyable_box<DecisionTree>(DecisionTree{DecisionTree::Node{
      .critical_value = next++, .lesser = std::move(l), .greater = std::move(g)}});
}

static DecisionTree::Node make_node(int child_depth, double& next) {
  auto l = make_tree(child_depth, next);
  auto g = make_tree(child_depth, next);
  return DecisionTree::Node{.critical_value = next++,
                            .lesser = std::move(l),
                            .greater = std::move(g)};
}

template <class T>
static inline void escape(T& x) {
  asm volatile("" : : "g"(&x) : "memory");
}

static inline void assign(Box& b, const Box& a) {
#ifdef WORKAROUND
  b = Box(a);
#else
  b = a;
#endif
}

static inline void assign(DecisionTree::Node& n2, const DecisionTree::Node& n1) {
#ifdef WORKAROUND
  n2 = DecisionTree::Node(n1);
#else
  n2 = n1;
#endif
}

template <class T>
static void run(const char* name, const T& src, T& dst, long iters) {
  // Allocations of one assignment, after a warm-up assignment.
  assign(dst, src);
  const std::size_t before = g_allocs;
  assign(dst, src);
  escape(dst);
  const std::size_t allocs = g_allocs - before;

  double best = 1e300;
  for (int sample = 0; sample < 15; ++sample) {
    const auto t0 = std::chrono::steady_clock::now();
    for (long i = 0; i < iters; ++i) {
      assign(dst, src);
      escape(dst);
    }
    const auto t1 = std::chrono::steady_clock::now();
    best = std::min(
        best, std::chrono::duration<double, std::nano>(t1 - t0).count() / iters);
  }
  std::printf("%s ns=%.1f allocs=%zu\n", name, best, allocs);
}

int main() {
  double next = 0.0;

  const Box leaf_a = make_tree(0, next);
  Box leaf_b = make_tree(0, next);
  run("box_leaf", leaf_a, leaf_b, 200000);

  const Box tree_a = make_tree(10, next);  // 2047 nodes
  Box tree_b = make_tree(10, next);
  run("box_tree", tree_a, tree_b, 300);

  const auto node_leaf_a = make_node(0, next);  // node + 2 leaves
  auto node_leaf_b = make_node(0, next);
  run("node_leaf", node_leaf_a, node_leaf_b, 200000);

  const auto node_tree_a = make_node(9, next);  // 1 + 2 * 1023 = 2047 nodes
  auto node_tree_b = make_node(9, next);
  run("node_tree", node_tree_a, node_tree_b, 300);
  return 0;
}

Merge Danger

Door: two-way

One line in a header. Reverting restores the old declaration.

Blast Radius: low

CopyableBox<T>, and structs that hold one, become copy-assignable. rfl::Box is unchanged, and so is moving a Box into a CopyableBox. The removed overload, assigning a non-copyable Box<T> to a CopyableBox<T>, never compiled when called: on main, copyable = noncopyable fails at if(this != &other) with "comparison of distinct pointer types" (Clang and GCC 16). Nothing that compiled before stops compiling. Only a trait such as std::is_assignable_v<rfl::CopyableBox<T>&, const rfl::Box<T>&> changes, from true to false. reflect-cpp itself checks no assignability traits.

🤖 Generated with Claude Code