Skip to content

Add move assignment operators to Statement and Attachment#36

Merged
asfernandes merged 1 commit into
asfernandes:mainfrom
fdcastel:statement-move
Feb 23, 2026
Merged

Add move assignment operators to Statement and Attachment#36
asfernandes merged 1 commit into
asfernandes:mainfrom
fdcastel:statement-move

Conversation

@fdcastel

Copy link
Copy Markdown
Contributor

Closes part of #23 (item 6 — Statement Move Assignment).

Motivation

Both Statement and Attachment had their move assignment operators explicitly deleted, even though move constructors were already supported. This prevented common C++ patterns:

  • std::swap(stmt1, stmt2)
  • Reassigning a Statement variable to a new prepared statement
  • Storing Statement or Attachment objects in containers that require move-assignability (e.g., std::vector grow/reallocate)

@asfernandes confirmed in #23: "It should be movable."

Changes

Internal: replace Client& / Attachment& reference members with pointers

Reference data members cannot be rebound after initialization, which is why move assignment was deleted. This PR changes the internal storage from references to pointers in the following classes:

Class Member changed Scope
Attachment Client& clientClient* client Private
Statement Attachment& attachmentAttachment* attachment Private
impl::StatusWrapper Client& clientClient* client Protected
impl::CalendarConverter Client& clientClient* client Private
impl::NumericConverter Client& clientClient* client Private

All public APIs remain unchanged — Attachment::getClient() still returns Client&.

New: Attachment::operator=(Attachment&&) noexcept

If the target Attachment owns a valid connection, it is disconnected first (matching the destructor's cleanup behavior). Then the handle is moved from the source.

New: Statement::operator=(Statement&&) noexcept

If the target Statement owns a valid handle, it is freed first (matching the destructor's cleanup behavior). Then all internal state is moved from the source.

Tests added

  • AttachmentSuite/moveAssignmentTransfersOwnership — creates two attachments, move-assigns one into the other, verifies ownership transfer and that the old connection was properly disconnected.
  • StatementLifecycleSuite/moveAssignmentTransfersOwnership — creates two statements, move-assigns, and verifies the moved-to statement can execute correctly.
  • StatementLifecycleSuite/moveAssignmentToMovedFromStatement — move-constructs a statement (leaving the source invalid), then move-assigns a different statement into the moved-from object, verifying it becomes valid and executable again.

Non-breaking

This is a purely additive change. Existing code that uses move construction, isValid(), or any other public API is unaffected.

Comment thread src/fb-cpp/Attachment.h Outdated
{
try
{
disconnectOrDrop(false);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the logic of move making the new instance invalid when the original was valid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The move assignment does not leave this invalid. It ends up valid with o's handle.

The disconnectOrDrop only cleans up the target's old connection before overwriting it. Here's the full sequence for a = std::move(b):

  1. disconnectOrDrop(false): calls detach() on a's old connection, then handle.reset()
  2. handle = std::move(o.handle): a takes ownership of b's handle

After: a is valid (with b's former connection), b is invalid (handle moved away).

The explicit detach() is necessary because FbRef::release() alone does not call detach() -- it only decrements the reference count. This mirrors the same cleanup pattern used in the destructor:

// destructor
~Attachment() noexcept
{
    if (isValid())
    {
        try { disconnectOrDrop(false); } catch (...) { /* swallow */ }
    }
}

Without the disconnectOrDrop, the old server-side connection would not be properly terminated.

I've updated the doxygen comment to make this clearer:

/// Same cleanup as the destructor: the current connection (if valid) is detached
/// before the handle is overwritten — FbRef::release() alone does not call detach().
/// After the assignment, this is valid (with o's handle) and o is invalid.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detach disconnects no mater how many refs are present, so we end it both old and new instances pointing to detached attachment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it now. You are right. I'm dumb. And Claude is on drugs 😄.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: removed the explicit disconnectOrDrop/free calls from both move assignment operators. Now the old handle is simply released via FbRef::operator=(FbRef&&), which calls release() to decrement the refcount — no server-side side-effects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

History rewritten. Please review when possible.

Replace internal Client& / Attachment& reference members with pointers
to enable rebinding in move assignment. This change affects:
- Attachment: Client& -> Client* (private)
- Statement: Attachment& -> Attachment* (private)
- impl::StatusWrapper: Client& -> Client* (protected)
- impl::CalendarConverter: Client& -> Client* (private)
- impl::NumericConverter: Client& -> Client* (private)

Add operator=(Statement&&) noexcept and operator=(Attachment&&) noexcept.
The old handles are released via FbRef::operator=(FbRef&&), which calls
release() to decrement the refcount without server-side side-effects.

Add tests:
- AttachmentSuite/moveAssignmentTransfersOwnership
- StatementLifecycleSuite/moveAssignmentTransfersOwnership
- StatementLifecycleSuite/moveAssignmentToMovedFromStatement

Relates to asfernandes#23 (item 6)
@asfernandes asfernandes merged commit 1da1d7b into asfernandes:main Feb 23, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants