Improve Cpp::Destruct to return success/failure of wrapper generation

include/CppInterOp/CppInterOp.h

@@ -838,14 +838,14 @@ enum : long int {
 CPPINTEROP_API std::vector<long int> 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);
 
@@ -865,7 +867,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

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);
 
 /**

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);
 }

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<Decl*>(scope);
-  Destruct(getInterp(), This, Class, withFree, count);
+  return Destruct(getInterp(), This, Class, withFree, count);
 }
 
 class StreamCaptureInfo {

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<int*>(reinterpret_cast<char*>(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");
@@ -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) {
@@ -2454,7 +2468,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 +2480,7 @@ TEST(FunctionReflectionTest, DestructArray) {
   // destruct the rest
   auto *new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(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 +2495,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();