Skip to content

Commit eed3e37

Browse files
Goober5000claude
andcommitted
fix geometry_batcher copy semantics and add move operations
clone() allocated fresh vert/radius_list buffers without freeing the ones already owned, so assigning over a live batcher leaked both. It also never copied buffer_offset, and the copy constructor called it with no member initialization at all -- so every copy-constructed batcher carried an uninitialized buffer_offset. Free the existing buffers in clone(), copy buffer_offset, and delegate the copy constructor to the default constructor so clone() sees initialized members (which is also what makes its new frees safe on that path). Also add noexcept move operations, which the user-declared copy operations had suppressed, so by-value handling can transfer the buffers instead of deep-copying them. None of these defects were reachable from current callers (the static batch maps mutate entries in place); this closes the latent hazards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 859b249 commit eed3e37

2 files changed

Lines changed: 48 additions & 2 deletions

File tree

code/graphics/grbatch.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
#include "graphics/grbatch.h"
1414
#include "render/3d.h"
1515

16+
#include <utility>
17+
1618
geometry_batcher::~geometry_batcher()
1719
{
1820
if (vert != NULL) {
@@ -124,6 +126,15 @@ void geometry_batcher::clone(const geometry_batcher &geo)
124126
n_to_render = geo.n_to_render;
125127
n_allocated = geo.n_allocated;
126128
use_radius = geo.use_radius;
129+
buffer_offset = geo.buffer_offset;
130+
131+
// free any buffers we already own, so that assignment doesn't leak them
132+
if (vert != nullptr) {
133+
vm_free(vert);
134+
}
135+
if (radius_list != nullptr) {
136+
vm_free(radius_list);
137+
}
127138

128139
if (n_allocated > 0) {
129140
vert = (vertex *) vm_malloc( sizeof(vertex) * n_allocated );
@@ -146,6 +157,37 @@ geometry_batcher& geometry_batcher::operator=(const geometry_batcher &geo)
146157
return *this;
147158
}
148159

160+
geometry_batcher::geometry_batcher(geometry_batcher &&other) noexcept
161+
: n_to_render(std::exchange(other.n_to_render, 0)),
162+
n_allocated(std::exchange(other.n_allocated, 0)),
163+
vert(std::exchange(other.vert, nullptr)),
164+
use_radius(std::exchange(other.use_radius, true)),
165+
radius_list(std::exchange(other.radius_list, nullptr)),
166+
buffer_offset(std::exchange(other.buffer_offset, -1))
167+
{
168+
}
169+
170+
geometry_batcher& geometry_batcher::operator=(geometry_batcher &&other) noexcept
171+
{
172+
if (this != &other) {
173+
if (vert != nullptr) {
174+
vm_free(vert);
175+
}
176+
if (radius_list != nullptr) {
177+
vm_free(radius_list);
178+
}
179+
180+
n_to_render = std::exchange(other.n_to_render, 0);
181+
n_allocated = std::exchange(other.n_allocated, 0);
182+
vert = std::exchange(other.vert, nullptr);
183+
use_radius = std::exchange(other.use_radius, true);
184+
radius_list = std::exchange(other.radius_list, nullptr);
185+
buffer_offset = std::exchange(other.buffer_offset, -1);
186+
}
187+
188+
return *this;
189+
}
190+
149191
/*
150192
0----1
151193
|\ |

code/graphics/grbatch.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,12 @@ class geometry_batcher
3232
geometry_batcher(): n_to_render(0), n_allocated(0), vert(NULL), use_radius(true), radius_list(NULL), buffer_offset(-1) {}
3333
~geometry_batcher();
3434

35-
geometry_batcher(const geometry_batcher &geo) { clone(geo); }
36-
geometry_batcher& operator=(const geometry_batcher &geo);
35+
// delegate to the default constructor so that clone() sees initialized members
36+
geometry_batcher(const geometry_batcher &geo) : geometry_batcher() { clone(geo); }
37+
geometry_batcher& operator=(const geometry_batcher &geo);
38+
39+
geometry_batcher(geometry_batcher &&other) noexcept;
40+
geometry_batcher& operator=(geometry_batcher &&other) noexcept;
3741

3842
// initial memory space needed
3943
// NOTE: This MUST be called BEFORE calling any of the draw_*() functions!!

0 commit comments

Comments
 (0)