From c6293db1d8d589004fa33a6fa0d1c995b0cc732d Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Mon, 30 Jun 2025 14:49:55 +0200 Subject: [PATCH 1/4] Improve Cpp::Destruct to indicate success/failure, adapt C-API and tests --- include/CppInterOp/CppInterOp.h | 3 ++- include/clang-c/CXCppInterOp.h | 4 +++- lib/CppInterOp/CXCppInterOp.cpp | 6 +++--- lib/CppInterOp/CppInterOp.cpp | 11 ++++++----- unittests/CppInterOp/FunctionReflectionTest.cpp | 14 +++++++------- 5 files changed, 21 insertions(+), 17 deletions(-) diff --git a/include/CppInterOp/CppInterOp.h b/include/CppInterOp/CppInterOp.h index 7b428e4b2..02b685947 100644 --- a/include/CppInterOp/CppInterOp.h +++ b/include/CppInterOp/CppInterOp.h @@ -865,7 +865,8 @@ CPPINTEROP_API TCppObject_t Construct(TCppScope_t scope, void* arena = nullptr, /// destructor /// \param[in] count indicate the number of objects to destruct, if \c This /// points to an array of objects -CPPINTEROP_API void Destruct(TCppObject_t This, TCppScope_t type, +/// \returns true if wrapper generation and invocation succeeded. +CPPINTEROP_API bool Destruct(TCppObject_t This, TCppScope_t type, bool withFree = true, TCppIndex_t count = 0UL); /// @name Stream Redirection diff --git a/include/clang-c/CXCppInterOp.h b/include/clang-c/CXCppInterOp.h index 638424ea8..ecd57262f 100644 --- a/include/clang-c/CXCppInterOp.h +++ b/include/clang-c/CXCppInterOp.h @@ -361,8 +361,10 @@ CINDEX_LINKAGE void clang_invoke(CXScope func, void* result, void** args, * \param type The type of the object. * * \param withFree Whether to call operator delete/free or not. + * + * \returns true if wrapper generation and invocation succeeded. */ -CINDEX_LINKAGE void clang_destruct(CXObject This, CXScope S, +CINDEX_LINKAGE bool clang_destruct(CXObject This, CXScope S, bool withFree = true, size_t nary = 0UL); /** diff --git a/lib/CppInterOp/CXCppInterOp.cpp b/lib/CppInterOp/CXCppInterOp.cpp index 0546f3a2f..8050a3b96 100644 --- a/lib/CppInterOp/CXCppInterOp.cpp +++ b/lib/CppInterOp/CXCppInterOp.cpp @@ -613,10 +613,10 @@ void clang_invoke(CXScope func, void* result, void** args, size_t n, } namespace Cpp { -void Destruct(compat::Interpreter& interp, TCppObject_t This, +bool Destruct(compat::Interpreter& interp, TCppObject_t This, clang::Decl* Class, bool withFree, size_t nary); } // namespace Cpp -void clang_destruct(CXObject This, CXScope S, bool withFree, size_t nary) { - Cpp::Destruct(*getInterpreter(S), This, getDecl(S), withFree, nary); +bool clang_destruct(CXObject This, CXScope S, bool withFree, size_t nary) { + return Cpp::Destruct(*getInterpreter(S), This, getDecl(S), withFree, nary); } diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index cd776dbce..3dcf49afa 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -3812,19 +3812,20 @@ TCppObject_t Construct(TCppScope_t scope, void* arena /*=nullptr*/, return Construct(getInterp(), scope, arena, count); } -void Destruct(compat::Interpreter& interp, TCppObject_t This, Decl* Class, +bool Destruct(compat::Interpreter& interp, TCppObject_t This, Decl* Class, bool withFree, TCppIndex_t nary) { if (auto wrapper = make_dtor_wrapper(interp, Class)) { (*wrapper)(This, nary, withFree); - return; + return true; } - // FIXME: Diagnose. + return false; + // FIXME: Enable stronger diagnostics } -void Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/, +bool Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/, TCppIndex_t count /*=0UL*/) { auto* Class = static_cast(scope); - Destruct(getInterp(), This, Class, withFree, count); + return Destruct(getInterp(), This, Class, withFree, count); } class StreamCaptureInfo { diff --git a/unittests/CppInterOp/FunctionReflectionTest.cpp b/unittests/CppInterOp/FunctionReflectionTest.cpp index 099e55aa2..a3c0c5292 100644 --- a/unittests/CppInterOp/FunctionReflectionTest.cpp +++ b/unittests/CppInterOp/FunctionReflectionTest.cpp @@ -1477,7 +1477,7 @@ TEST(FunctionReflectionTest, JitCallAdvanced) { Ctor.Invoke(&object); EXPECT_TRUE(object) << "Failed to call the ctor."; // Building a wrapper with a typedef decl must be possible. - Cpp::Destruct(object, Decls[1]); + EXPECT_TRUE(Cpp::Destruct(object, Decls[1])); // C API auto* I = clang_createInterpreterFromRawPtr(Cpp::GetInterpreter()); @@ -2342,7 +2342,7 @@ TEST(FunctionReflectionTest, ConstructArray) { obj = reinterpret_cast(reinterpret_cast(where) + (Cpp::SizeOf(scope) * 4)); EXPECT_TRUE(*obj == 42); - Cpp::Destruct(where, scope, /*withFree=*/false, 5); + EXPECT_TRUE(Cpp::Destruct(where, scope, /*withFree=*/false, 5)); Cpp::Deallocate(scope, where, 5); output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output, @@ -2379,7 +2379,7 @@ TEST(FunctionReflectionTest, Destruct) { testing::internal::CaptureStdout(); Cpp::TCppScope_t scope = Cpp::GetNamed("C"); Cpp::TCppObject_t object = Cpp::Construct(scope); - Cpp::Destruct(object, scope); + EXPECT_TRUE(Cpp::Destruct(object, scope)); std::string output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output, "Destructor Executed"); @@ -2389,7 +2389,7 @@ TEST(FunctionReflectionTest, Destruct) { object = Cpp::Construct(scope); // Make sure we do not call delete by adding an explicit Deallocate. If we // called delete the Deallocate will cause a double deletion error. - Cpp::Destruct(object, scope, /*withFree=*/false); + EXPECT_TRUE(Cpp::Destruct(object, scope, /*withFree=*/false)); Cpp::Deallocate(scope, object); output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output, "Destructor Executed"); @@ -2454,7 +2454,7 @@ TEST(FunctionReflectionTest, DestructArray) { testing::internal::CaptureStdout(); // destruct 3 out of 5 objects - Cpp::Destruct(where, scope, false, 3); + EXPECT_TRUE(Cpp::Destruct(where, scope, false, 3)); output = testing::internal::GetCapturedStdout(); EXPECT_EQ( @@ -2466,7 +2466,7 @@ TEST(FunctionReflectionTest, DestructArray) { // destruct the rest auto *new_head = reinterpret_cast(reinterpret_cast(where) + (Cpp::SizeOf(scope) * 3)); - Cpp::Destruct(new_head, scope, false, 2); + EXPECT_TRUE(Cpp::Destruct(new_head, scope, false, 2)); output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output, "\nDestructor Executed\n\nDestructor Executed\n"); @@ -2481,7 +2481,7 @@ TEST(FunctionReflectionTest, DestructArray) { testing::internal::CaptureStdout(); // FIXME : This should work with the array of objects as well // Cpp::Destruct(where, scope, true, 5); - Cpp::Destruct(where, scope, true); + EXPECT_TRUE(Cpp::Destruct(where, scope, true)); output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output, "\nDestructor Executed\n"); output.clear(); From c016444e81caf280b995cd636b7ab35ef939754b Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Tue, 1 Jul 2025 08:21:10 +0200 Subject: [PATCH 2/4] Add negative case test for codegen failure in Cpp::Destruct --- unittests/CppInterOp/FunctionReflectionTest.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/unittests/CppInterOp/FunctionReflectionTest.cpp b/unittests/CppInterOp/FunctionReflectionTest.cpp index a3c0c5292..d5cc81e78 100644 --- a/unittests/CppInterOp/FunctionReflectionTest.cpp +++ b/unittests/CppInterOp/FunctionReflectionTest.cpp @@ -2406,6 +2406,20 @@ TEST(FunctionReflectionTest, Destruct) { // Clean up resources clang_Interpreter_takeInterpreterAsPtr(I); clang_Interpreter_dispose(I); + + // Failure test, this wrapper should not compile since we explicitly delete + // the destructor + Interp->declare(R"( + class D { + public: + D() {} + ~D() = delete; + }; + )"); + + scope = Cpp::GetNamed("D"); + object = Cpp::Construct(scope); + EXPECT_FALSE(Cpp::Destruct(object, scope)); } TEST(FunctionReflectionTest, DestructArray) { From bd0f9a3561d628f2cc078c3dbc1ec63f63be2f8f Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Mon, 30 Jun 2025 14:52:14 +0200 Subject: [PATCH 3/4] Improve documentation of memory allocation/deallocation API --- include/CppInterOp/CppInterOp.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/include/CppInterOp/CppInterOp.h b/include/CppInterOp/CppInterOp.h index 02b685947..a47d3a372 100644 --- a/include/CppInterOp/CppInterOp.h +++ b/include/CppInterOp/CppInterOp.h @@ -838,14 +838,14 @@ enum : long int { CPPINTEROP_API std::vector GetDimensions(TCppType_t type); /// Allocates memory required by an object of a given class -/// \c scope Given class for which to allocate memory for -/// \c count is used to indicate the number of objects to allocate for. +/// \param[in] scope Given class for which to allocate memory for +/// \param[in] count is used to indicate the number of objects to allocate for. CPPINTEROP_API TCppObject_t Allocate(TCppScope_t scope, TCppIndex_t count = 1UL); /// Deallocates memory for a given class. -/// \c scope Class to indicate size of memory to deallocate -/// \c count is used to indicate the number of objects to dallocate for +/// \param[in] scope Class to indicate size of memory to deallocate +/// \param[in] count is used to indicate the number of objects to dallocate for CPPINTEROP_API void Deallocate(TCppScope_t scope, TCppObject_t address, TCppIndex_t count = 1UL); @@ -855,6 +855,8 @@ CPPINTEROP_API void Deallocate(TCppScope_t scope, TCppObject_t address, /// \param[in] arena If set, this API uses placement new to construct at this /// address. /// \param[in] is used to indicate the number of objects to construct. +/// \returns a pointer to the constructed object, which is arena if placement +/// new is used. CPPINTEROP_API TCppObject_t Construct(TCppScope_t scope, void* arena = nullptr, TCppIndex_t count = 1UL); From 84043510fe1a86fe6dd62576087b1b241e509530 Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Tue, 1 Jul 2025 09:54:10 +0200 Subject: [PATCH 4/4] Add const-correctness to the scope of destruct call --- include/CppInterOp/CppInterOp.h | 3 ++- lib/CppInterOp/CXCppInterOp.cpp | 2 +- lib/CppInterOp/CppInterOp.cpp | 8 ++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/CppInterOp/CppInterOp.h b/include/CppInterOp/CppInterOp.h index a47d3a372..534a8f45c 100644 --- a/include/CppInterOp/CppInterOp.h +++ b/include/CppInterOp/CppInterOp.h @@ -35,6 +35,7 @@ namespace Cpp { using TCppIndex_t = size_t; using TCppScope_t = void*; +using TCppConstScope_t = const void*; using TCppType_t = void*; using TCppFunction_t = void*; using TCppConstFunction_t = const void*; @@ -868,7 +869,7 @@ CPPINTEROP_API TCppObject_t Construct(TCppScope_t scope, void* arena = nullptr, /// \param[in] count indicate the number of objects to destruct, if \c This /// points to an array of objects /// \returns true if wrapper generation and invocation succeeded. -CPPINTEROP_API bool Destruct(TCppObject_t This, TCppScope_t type, +CPPINTEROP_API bool Destruct(TCppObject_t This, TCppConstScope_t type, bool withFree = true, TCppIndex_t count = 0UL); /// @name Stream Redirection diff --git a/lib/CppInterOp/CXCppInterOp.cpp b/lib/CppInterOp/CXCppInterOp.cpp index 8050a3b96..bd9043d5b 100644 --- a/lib/CppInterOp/CXCppInterOp.cpp +++ b/lib/CppInterOp/CXCppInterOp.cpp @@ -614,7 +614,7 @@ void clang_invoke(CXScope func, void* result, void** args, size_t n, namespace Cpp { bool Destruct(compat::Interpreter& interp, TCppObject_t This, - clang::Decl* Class, bool withFree, size_t nary); + const clang::Decl* Class, bool withFree, size_t nary); } // namespace Cpp bool clang_destruct(CXObject This, CXScope S, bool withFree, size_t nary) { diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index 3dcf49afa..b4dd7ad74 100755 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/lib/CppInterOp/CppInterOp.cpp @@ -3812,7 +3812,7 @@ TCppObject_t Construct(TCppScope_t scope, void* arena /*=nullptr*/, return Construct(getInterp(), scope, arena, count); } -bool Destruct(compat::Interpreter& interp, TCppObject_t This, Decl* Class, +bool Destruct(compat::Interpreter& interp, TCppObject_t This, const Decl* Class, bool withFree, TCppIndex_t nary) { if (auto wrapper = make_dtor_wrapper(interp, Class)) { (*wrapper)(This, nary, withFree); @@ -3822,9 +3822,9 @@ bool Destruct(compat::Interpreter& interp, TCppObject_t This, Decl* Class, // FIXME: Enable stronger diagnostics } -bool Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/, - TCppIndex_t count /*=0UL*/) { - auto* Class = static_cast(scope); +bool Destruct(TCppObject_t This, TCppConstScope_t scope, + bool withFree /*=true*/, TCppIndex_t count /*=0UL*/) { + const auto* Class = static_cast(scope); return Destruct(getInterp(), This, Class, withFree, count); }