Skip to content

Commit b312db9

Browse files
committed
fix(pine-cpp): explicitly delete RowFrame / ColumnFrame copy & move (#135 review)
Address bot-reviewer concern on the previous PR commits: switching mu_ from `std::shared_mutex` to `shared_ptr<std::shared_mutex>` removed the implicit non-copyable/non-movable guarantee that the inline shared_mutex member used to provide. Every other RowFrame member is copyable, so post-migration RowFrame became implicitly copy/move-constructible. A value copy would have aliased the mutex (shared_ptr copy) while deep-copying items_ — two "independent" frames sharing one lock for unrelated data, exactly the kind of subtle hazard #103 was about preventing. ColumnFrame stayed non-copyable through its `unique_ptr<ColumnStore>`, producing the asymmetry the reviewer flagged. Fix: explicitly delete copy/move ctors and assignments on both RowFrame and ColumnFrame. RowFrame restores the lost invariant; ColumnFrame pins the implicit one explicitly so the contract is self-describing rather than depending on a future reader noticing the unique_ptr member. No call site copies or moves either Frame in the current tree; this commit is purely about preventing future misuse. Test suite still passes (213/213, 110k+ assertions) — no value-copy paths existed to break. Refs #103, #135.
1 parent 6d90a2b commit b312db9

2 files changed

Lines changed: 22 additions & 0 deletions

File tree

pine-cpp/include/pine/column_frame.hpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@ class ColumnFrame : public Frame {
3131
ColumnFrame();
3232
ColumnFrame(Variant::object_t common, std::vector<Variant::object_t> items);
3333

34+
// Non-copyable, non-movable. unique_ptr<ColumnStore> already enforces
35+
// this implicitly, but pin it explicitly to mirror RowFrame's
36+
// post-#103 declaration and keep the contract self-describing.
37+
ColumnFrame(const ColumnFrame&) = delete;
38+
ColumnFrame& operator=(const ColumnFrame&) = delete;
39+
ColumnFrame(ColumnFrame&&) = delete;
40+
ColumnFrame& operator=(ColumnFrame&&) = delete;
41+
3442
// Static convenience constructor kept for callers that already know
3543
// they want a ColumnFrame window view. parallel_execute uses the
3644
// virtual Frame::make_window_view on the parent — this static form

pine-cpp/include/pine/row_frame.hpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,20 @@ class RowFrame : public Frame {
2828
RowFrame();
2929
RowFrame(Variant::object_t common, std::vector<Variant::object_t> items);
3030

31+
// Non-copyable, non-movable. Pre-#103 this was implicit (the inline
32+
// std::shared_mutex member was itself neither copyable nor movable);
33+
// after the migration to shared_ptr<shared_mutex> the implicit
34+
// protection was lost — every other member is copyable, so RowFrame
35+
// would have become copy/move-constructible. A value copy would alias
36+
// the mutex (shared_ptr copy) but deep-copy items_ storage, leaving
37+
// two "independent" frames sharing one lock for unrelated data.
38+
// ColumnFrame stays non-copyable via its unique_ptr<ColumnStore>; we
39+
// pin RowFrame the same way explicitly to keep the symmetry.
40+
RowFrame(const RowFrame&) = delete;
41+
RowFrame& operator=(const RowFrame&) = delete;
42+
RowFrame(RowFrame&&) = delete;
43+
RowFrame& operator=(RowFrame&&) = delete;
44+
3145
// ---- Frame interface ----
3246
Variant common(const std::string& field) const override;
3347
bool has_common(const std::string& field) const override;

0 commit comments

Comments
 (0)