Skip to content

Improve Cpp::Construct pointer logic for memory arena flags #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/CppInterOp/CppInterOp.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,8 @@ class JitCall {
///\param[in] nary - Use array new if we have to construct an array of
/// objects (nary > 1).
///\param[in] args - a pointer to a argument list and argument size.
///\param[in] is_arena - a pointer that indicates if placement new is to be
/// used
// FIXME: Change the type of withFree from int to bool in the wrapper code.
void InvokeConstructor(void* result, unsigned long nary = 1,
ArgList args = {}, void* is_arena = nullptr) const {
Expand Down Expand Up @@ -849,7 +851,7 @@ CPPINTEROP_API void Deallocate(TCppScope_t scope, TCppObject_t address,

/// Creates one or more objects of class \c scope by calling its default
/// constructor.
/// \param[in] scope Class to construct
/// \param[in] scope Class to construct, or handle to Constructor
/// \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.
Expand Down
31 changes: 17 additions & 14 deletions lib/CppInterOp/CppInterOp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3784,22 +3784,25 @@ void Deallocate(TCppScope_t scope, TCppObject_t address, TCppIndex_t count) {
// FIXME: Add optional arguments to the operator new.
TCppObject_t Construct(compat::Interpreter& interp, TCppScope_t scope,
void* arena /*=nullptr*/, TCppIndex_t count /*=1UL*/) {
auto* Class = (Decl*)scope;
// FIXME: Diagnose.
if (!HasDefaultConstructor(Class))
return nullptr;

auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.InvokeConstructor(&arena, count, {},
(void*)~0); // Tell Invoke to use placement new.
return arena;
}
if (!Cpp::IsConstructor(scope) && !Cpp::IsClass(scope))
return nullptr;
if (Cpp::IsClass(scope) && !HasDefaultConstructor(scope))
return nullptr;

void* obj = nullptr;
JC.InvokeConstructor(&obj, count, {}, nullptr);
return obj;
TCppFunction_t ctor = nullptr;
if (Cpp::IsClass(scope))
ctor = Cpp::GetDefaultConstructor(scope);
else // a ctor
ctor = scope;

if (JitCall JC = MakeFunctionCallable(&interp, ctor)) {
// invoke the constructor (placement/heap) in one shot
// flag is non-null for placement new, null for normal new
void* is_arena = arena ? reinterpret_cast<void*>(1) : nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

    void* is_arena = arena ? reinterpret_cast<void*>(1) : nullptr;
                             ^

void* result = arena;
JC.InvokeConstructor(&result, count, /*args=*/{}, is_arena);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

    JC.InvokeConstructor(&result, count, /*args=*/{}, is_arena);
                         ^

return result;
}
return nullptr;
}
Expand Down
24 changes: 21 additions & 3 deletions unittests/CppInterOp/FunctionReflectionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2122,20 +2122,24 @@ TEST(FunctionReflectionTest, Construct) {
GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
#endif
std::vector<const char*> interpreter_args = {"-include", "new"};
Cpp::CreateInterpreter(interpreter_args);
std::vector<Decl*> Decls, SubDecls;

Interp->declare(R"(
std::string code = R"(
#include <new>
extern "C" int printf(const char*,...);
class C {
public:
int x;
C() {
x = 12345;
printf("Constructor Executed");
}
};
)");
void construct() { return; }
)";

GetAllTopLevelDecls(code, Decls, false, interpreter_args);
GetAllSubDecls(Decls[1], SubDecls);
testing::internal::CaptureStdout();
Cpp::TCppScope_t scope = Cpp::GetNamed("C");
Cpp::TCppObject_t object = Cpp::Construct(scope);
Expand All @@ -2155,6 +2159,20 @@ TEST(FunctionReflectionTest, Construct) {
EXPECT_EQ(output, "Constructor Executed");
output.clear();

// Pass a constructor
testing::internal::CaptureStdout();
where = Cpp::Allocate(scope);
EXPECT_TRUE(where == Cpp::Construct(SubDecls[3], where));
EXPECT_TRUE(*(int*)where == 12345);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

  EXPECT_TRUE(*(int*)where == 12345);
               ^

Cpp::Deallocate(scope, where);
output = testing::internal::GetCapturedStdout();
EXPECT_EQ(output, "Constructor Executed");
output.clear();

// Pass a non-class decl, this should fail
where = Cpp::Allocate(scope);
where = Cpp::Construct(Decls[2], where);
EXPECT_TRUE(where == nullptr);
// C API
testing::internal::CaptureStdout();
auto* I = clang_createInterpreterFromRawPtr(Cpp::GetInterpreter());
Expand Down
Loading