From c7cf46bdeab18e941acdd647ec4adbed2957f07e Mon Sep 17 00:00:00 2001 From: Ivan FB <128452529+Ivansete-status@users.noreply.github.com> Date: Thu, 21 May 2026 16:38:13 +0200 Subject: [PATCH] avoid move ctor and assing operator in cpp generated code (#36) --- examples/timer/cpp_bindings/main.cpp | 10 ++--- examples/timer/cpp_bindings/my_timer.hpp | 42 +++++++------------ ffi/codegen/cpp.nim | 14 +++++-- .../templates/cpp/context_rule_of_5.hpp.tpl | 36 +++++----------- tests/e2e/cpp/test_timer_e2e.cpp | 25 +++++------ 5 files changed, 54 insertions(+), 73 deletions(-) diff --git a/examples/timer/cpp_bindings/main.cpp b/examples/timer/cpp_bindings/main.cpp index 5a7e3c8..739ad9f 100644 --- a/examples/timer/cpp_bindings/main.cpp +++ b/examples/timer/cpp_bindings/main.cpp @@ -7,9 +7,9 @@ int main() { auto ctx = MyTimerCtx::create(TimerConfig{"cpp-demo"}); std::cout << "[1] Context created\n"; - auto versionFuture = ctx.versionAsync(); - auto echo1Future = ctx.echoAsync(EchoRequest{"hello from C++", 200}); - auto echo2Future = ctx.echoAsync(EchoRequest{"second C++ request", 50}); + auto versionFuture = ctx->versionAsync(); + auto echo1Future = ctx->echoAsync(EchoRequest{"hello from C++", 200}); + auto echo2Future = ctx->echoAsync(EchoRequest{"second C++ request", 50}); auto version = versionFuture.get(); std::cout << "[2] Version: " << version << "\n"; @@ -29,7 +29,7 @@ int main() { std::optional(3) }; - auto complexFuture = ctx.complexAsync(complexReq); + auto complexFuture = ctx->complexAsync(complexReq); auto complex = complexFuture.get(); std::cout << "[5] Complex: summary=" << complex.summary << ", itemCount=" << complex.itemCount @@ -55,7 +55,7 @@ int main() { /*jitter*/ std::optional(250), }; - auto scheduleFuture = ctx.scheduleAsync(job, retry, schedule); + auto scheduleFuture = ctx->scheduleAsync(job, retry, schedule); auto scheduleRes = scheduleFuture.get(); std::cout << "[6] Schedule (3 complex params): jobId=" << scheduleRes.jobId << ", willRunCount=" << scheduleRes.willRunCount diff --git a/examples/timer/cpp_bindings/my_timer.hpp b/examples/timer/cpp_bindings/my_timer.hpp index 93173cd..e403760 100644 --- a/examples/timer/cpp_bindings/my_timer.hpp +++ b/examples/timer/cpp_bindings/my_timer.hpp @@ -661,7 +661,7 @@ inline std::vector ffi_call_(std::function create(const TimerConfig& config, std::chrono::milliseconds timeout = std::chrono::seconds{30}) { const auto ffi_req_ = MyTimerCreateCtorReq{config}; const auto ffi_req_bytes_ = encodeCborFFI(ffi_req_); const auto ffi_raw_ = ffi_call_([&](FFICallback cb, void* ud) { @@ -671,28 +671,25 @@ public: const auto addr_str = decodeCborFFI(ffi_raw_); try { const auto addr = std::stoull(addr_str); - return MyTimerCtx(reinterpret_cast(static_cast(addr)), timeout); + return std::unique_ptr(new MyTimerCtx(reinterpret_cast(static_cast(addr)), timeout)); } catch (const std::exception&) { throw std::runtime_error("FFI create returned non-numeric address: " + addr_str); } } - static std::future createAsync(const TimerConfig& config, std::chrono::milliseconds timeout = std::chrono::seconds{30}) { + static std::future> createAsync(const TimerConfig& config, std::chrono::milliseconds timeout = std::chrono::seconds{30}) { return std::async(std::launch::async, [config, timeout]() { return create(config, timeout); }); } - // Rule of Five: because this class owns a raw resource (the my_timer - // context pointer freed in the destructor), the compiler-generated copy - // and move special members would do the wrong thing — copies would - // double-free, and a default move would leave both objects pointing at - // the same context. So we define all five special members explicitly: - // 1. destructor — releases the context. - // 2. copy constructor — deleted; contexts are not copyable. - // 3. copy assignment — deleted; same reason. - // 4. move constructor — transfers ownership, nulls the source. - // 5. move assignment — destroys the current context, then - // transfers ownership from `other`. - // See: https://en.cppreference.com/w/cpp/language/rule_of_three + // Special-member policy: this class owns a my_timer context, which in + // turn owns the library's worker thread(s) and internal state. Moving + // such an object out from under a caller silently tears that state + // down and is easy to misuse (e.g. storing in a container that + // relocates its elements). It also has no clean analogue in the other + // binding languages we generate. So copies and moves are both + // deleted; ownership is transferred via MyTimerCtx::create returning a + // std::unique_ptr. The destructor still releases the + // context. ~MyTimerCtx() { if (ptr_) { my_timer_destroy(ptr_); @@ -702,19 +699,8 @@ public: MyTimerCtx(const MyTimerCtx&) = delete; MyTimerCtx& operator=(const MyTimerCtx&) = delete; - - MyTimerCtx(MyTimerCtx&& other) noexcept : ptr_(other.ptr_), timeout_(other.timeout_) { - other.ptr_ = nullptr; - } - MyTimerCtx& operator=(MyTimerCtx&& other) noexcept { - if (this != &other) { - if (ptr_) my_timer_destroy(ptr_); - ptr_ = other.ptr_; - timeout_ = other.timeout_; - other.ptr_ = nullptr; - } - return *this; - } + MyTimerCtx(MyTimerCtx&&) = delete; + MyTimerCtx& operator=(MyTimerCtx&&) = delete; EchoResponse echo(const EchoRequest& req) const { const auto ffi_req_ = MyTimerEchoReq{req}; diff --git a/ffi/codegen/cpp.nim b/ffi/codegen/cpp.nim index fca69e9..e6b4d7b 100644 --- a/ffi/codegen/cpp.nim +++ b/ffi/codegen/cpp.nim @@ -282,7 +282,14 @@ proc generateCppHeader*( # path anyway since it carries the CBOR-encoded ctx address. Discard the # synchronous return and yield 0 from the lambda; the address comes back # through the callback's CBOR text-string payload. - lines.add(" static $1 create($2) {" % [ctxTypeName, ctorParamsWithTimeout]) + # `create` returns std::unique_ptr rather than a Ctx by value: the + # context owns library threads, so we forbid copy/move on the class + # itself (see ContextRuleOf5Tpl) and hand out ownership through a + # smart pointer that callers can move, store in containers, etc. + lines.add( + " static std::unique_ptr<$1> create($2) {" % + [ctxTypeName, ctorParamsWithTimeout] + ) lines.add(" const auto ffi_req_ = $1;" % [reqInit]) lines.add(" const auto ffi_req_bytes_ = encodeCborFFI(ffi_req_);") lines.add(" const auto ffi_raw_ = ffi_call_([&](FFICallback cb, void* ud) {") @@ -295,8 +302,9 @@ proc generateCppHeader*( lines.add(" const auto addr_str = decodeCborFFI(ffi_raw_);") lines.add(" try {") lines.add(" const auto addr = std::stoull(addr_str);") + # Use `new` directly (not std::make_unique) so the ctor can stay private. lines.add( - " return $1(reinterpret_cast(static_cast(addr)), timeout);" % + " return std::unique_ptr<$1>(new $1(reinterpret_cast(static_cast(addr)), timeout));" % [ctxTypeName] ) lines.add(" } catch (const std::exception&) {") @@ -318,7 +326,7 @@ proc generateCppHeader*( else: "timeout" lines.add( - " static std::future<$1> createAsync($2) {" % + " static std::future> createAsync($2) {" % [ctxTypeName, ctorParamsWithTimeout] ) lines.add( diff --git a/ffi/codegen/templates/cpp/context_rule_of_5.hpp.tpl b/ffi/codegen/templates/cpp/context_rule_of_5.hpp.tpl index d09405b..22ce19d 100644 --- a/ffi/codegen/templates/cpp/context_rule_of_5.hpp.tpl +++ b/ffi/codegen/templates/cpp/context_rule_of_5.hpp.tpl @@ -1,15 +1,12 @@ - // Rule of Five: because this class owns a raw resource (the {{LIB}} - // context pointer freed in the destructor), the compiler-generated copy - // and move special members would do the wrong thing — copies would - // double-free, and a default move would leave both objects pointing at - // the same context. So we define all five special members explicitly: - // 1. destructor — releases the context. - // 2. copy constructor — deleted; contexts are not copyable. - // 3. copy assignment — deleted; same reason. - // 4. move constructor — transfers ownership, nulls the source. - // 5. move assignment — destroys the current context, then - // transfers ownership from `other`. - // See: https://en.cppreference.com/w/cpp/language/rule_of_three + // Special-member policy: this class owns a {{LIB}} context, which in + // turn owns the library's worker thread(s) and internal state. Moving + // such an object out from under a caller silently tears that state + // down and is easy to misuse (e.g. storing in a container that + // relocates its elements). It also has no clean analogue in the other + // binding languages we generate. So copies and moves are both + // deleted; ownership is transferred via {{CTX}}::create returning a + // std::unique_ptr<{{CTX}}>. The destructor still releases the + // context. ~{{CTX}}() { if (ptr_) { {{LIB}}_destroy(ptr_); @@ -19,16 +16,5 @@ {{CTX}}(const {{CTX}}&) = delete; {{CTX}}& operator=(const {{CTX}}&) = delete; - - {{CTX}}({{CTX}}&& other) noexcept : ptr_(other.ptr_), timeout_(other.timeout_) { - other.ptr_ = nullptr; - } - {{CTX}}& operator=({{CTX}}&& other) noexcept { - if (this != &other) { - if (ptr_) {{LIB}}_destroy(ptr_); - ptr_ = other.ptr_; - timeout_ = other.timeout_; - other.ptr_ = nullptr; - } - return *this; - } + {{CTX}}({{CTX}}&&) = delete; + {{CTX}}& operator=({{CTX}}&&) = delete; diff --git a/tests/e2e/cpp/test_timer_e2e.cpp b/tests/e2e/cpp/test_timer_e2e.cpp index 4941f63..ffed1ec 100644 --- a/tests/e2e/cpp/test_timer_e2e.cpp +++ b/tests/e2e/cpp/test_timer_e2e.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -19,7 +20,7 @@ namespace { -MyTimerCtx makeCtx(const std::string& name = "e2e") { +std::unique_ptr makeCtx(const std::string& name = "e2e") { return MyTimerCtx::create(TimerConfig{name}); } @@ -34,19 +35,19 @@ TEST(TimerE2E, CreateAndDestroy) { TEST(TimerE2E, VersionSync) { auto ctx = makeCtx("version-sync"); - const auto v = ctx.version(); + const auto v = ctx->version(); EXPECT_EQ(v, "nim-timer v0.1.0"); } TEST(TimerE2E, VersionAsync) { auto ctx = makeCtx("version-async"); - auto fut = ctx.versionAsync(); + auto fut = ctx->versionAsync(); EXPECT_EQ(fut.get(), "nim-timer v0.1.0"); } TEST(TimerE2E, EchoRoundTripsMessageAndTimerName) { auto ctx = makeCtx("echo-ctx"); - const auto resp = ctx.echo(EchoRequest{"hello", 10}); + const auto resp = ctx->echo(EchoRequest{"hello", 10}); EXPECT_EQ(resp.echoed, "hello"); EXPECT_EQ(resp.timerName, "echo-ctx"); } @@ -56,7 +57,7 @@ TEST(TimerE2E, EchoHonoursDelay) { constexpr int delayMs = 150; const auto start = std::chrono::steady_clock::now(); - const auto resp = ctx.echo(EchoRequest{"waited", delayMs}); + const auto resp = ctx->echo(EchoRequest{"waited", delayMs}); const auto elapsed = std::chrono::duration_cast( std::chrono::steady_clock::now() - start).count(); @@ -68,9 +69,9 @@ TEST(TimerE2E, EchoHonoursDelay) { TEST(TimerE2E, ConcurrentAsyncCallsAreIndependent) { auto ctx = makeCtx("concurrent"); - auto f1 = ctx.echoAsync(EchoRequest{"one", 80}); - auto f2 = ctx.echoAsync(EchoRequest{"two", 40}); - auto f3 = ctx.echoAsync(EchoRequest{"three", 20}); + auto f1 = ctx->echoAsync(EchoRequest{"one", 80}); + auto f2 = ctx->echoAsync(EchoRequest{"two", 40}); + auto f3 = ctx->echoAsync(EchoRequest{"three", 20}); const auto r3 = f3.get(); const auto r2 = f2.get(); @@ -93,7 +94,7 @@ TEST(TimerE2E, ComplexWithOptionalNotePresent) { std::optional(2), }; - const auto resp = ctx.complex(req); + const auto resp = ctx->complex(req); EXPECT_EQ(resp.itemCount, 2); EXPECT_TRUE(resp.hasNote); EXPECT_NE(resp.summary.find("note=a note"), std::string::npos) @@ -111,7 +112,7 @@ TEST(TimerE2E, ComplexWithOptionalNoteAbsent) { std::nullopt, }; - const auto resp = ctx.complex(req); + const auto resp = ctx->complex(req); EXPECT_EQ(resp.itemCount, 0); EXPECT_FALSE(resp.hasNote); EXPECT_NE(resp.summary.find("note="), std::string::npos) @@ -124,8 +125,8 @@ TEST(TimerE2E, IndependentContextsKeepTheirOwnState) { auto ctxA = makeCtx("alpha"); auto ctxB = makeCtx("beta"); - const auto rA = ctxA.echo(EchoRequest{"x", 5}); - const auto rB = ctxB.echo(EchoRequest{"x", 5}); + const auto rA = ctxA->echo(EchoRequest{"x", 5}); + const auto rB = ctxB->echo(EchoRequest{"x", 5}); EXPECT_EQ(rA.timerName, "alpha"); EXPECT_EQ(rB.timerName, "beta");