Skip to content

Add support for array of objects in Construct/Destruct #587

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented May 21, 2025

No description provided.

@aaronj0 aaronj0 requested a review from vgvassilev May 21, 2025 15:37
Copy link

codecov bot commented May 21, 2025

Codecov Report

Attention: Patch coverage is 72.41379% with 24 lines in your changes missing coverage. Please review.

Project coverage is 77.79%. Comparing base (e05026a) to head (7596735).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 65.71% 24 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
+ Coverage   77.66%   77.79%   +0.12%     
==========================================
  Files           9        9              
  Lines        3743     3787      +44     
==========================================
+ Hits         2907     2946      +39     
- Misses        836      841       +5     
Files with missing lines Coverage Δ
include/CppInterOp/CppInterOp.h 100.00% <100.00%> (ø)
lib/CppInterOp/CXCppInterOp.cpp 48.86% <100.00%> (ø)
lib/CppInterOp/CppInterOp.cpp 85.10% <65.71%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
include/CppInterOp/CppInterOp.h 100.00% <100.00%> (ø)
lib/CppInterOp/CXCppInterOp.cpp 48.86% <100.00%> (ø)
lib/CppInterOp/CppInterOp.cpp 85.10% <65.71%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aaronj0 aaronj0 marked this pull request as draft May 21, 2025 16:47
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 22. Check the log or trigger a new build to see more.

// Forward if we intended to call a dtor with only 1 parameter.
if (m_Kind == kDestructorCall && result && !args.m_Args)
return InvokeDestructor(result, /*nary=*/0UL, /*withFree=*/true);
return InvokeDestructor(result, nary, /*withFree=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]

Suggested change
return InvokeDestructor(result, nary, /*withFree=*/true);
{ InvokeDestructor(result, nary, /*withFree=*/true); return;
}


#ifndef NDEBUG
assert(AreArgumentsValid(result, args, self) && "Invalid args!");
ReportInvokeStart(result, args, self);
#endif // NDEBUG
m_GenericCall(self, args.m_ArgSize, args.m_Args, result);
m_GenericCall(self, args.m_ArgSize, args.m_Args, result, nary);
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 access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

      m_GenericCall(self, args.m_ArgSize, args.m_Args, result, nary);
      ^


#ifndef NDEBUG
assert(AreArgumentsValid(result, args, self) && "Invalid args!");
ReportInvokeStart(result, args, self);
#endif // NDEBUG
m_GenericCall(self, args.m_ArgSize, args.m_Args, result);
m_GenericCall(self, args.m_ArgSize, args.m_Args, result, nary);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

      m_GenericCall(self, args.m_ArgSize, args.m_Args, result, nary);
                          ^

TCppObject_t Allocate(TCppScope_t scope) {
return (TCppObject_t)::operator new(Cpp::SizeOf(scope));
TCppObject_t Allocate(TCppScope_t scope, TCppIndex_t count) {
return (TCppObject_t)::operator new(Cpp::SizeOf(scope) * count);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "operator new" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOp.cpp:39:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <new>
+ #if CLANG_VERSION_MAJOR >= 19

::operator delete(address);
void Deallocate(TCppScope_t scope, TCppObject_t address, TCppIndex_t count) {
size_t bytes = Cpp::SizeOf(scope) * count;
::operator delete(address, bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "operator delete" is directly included [misc-include-cleaner]

    ::operator delete(address, bytes);
      ^

@@ -3596,7 +3633,8 @@
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.Invoke(&arena, {}, (void*)~0,
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]

        JC.Invoke(&arena, {}, (void*)~0,
                              ^

@@ -3596,7 +3633,8 @@
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.Invoke(&arena, {}, (void*)~0,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

        JC.Invoke(&arena, {}, (void*)~0,
                              ^

@@ -3596,7 +3633,8 @@
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.Invoke(&arena, {}, (void*)~0,
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.Invoke(&arena, {}, (void*)~0,
                  ^

@@ -1950,6 +1950,58 @@ TEST(FunctionReflectionTest, ConstructNested) {
output.clear();
}

TEST(FunctionReflectionTest, ConstructArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
TEST(FunctionReflectionTest, ConstructArray) {
TESTstatic (FunctionReflectionTest, ConstructArray) {

testing::internal::CaptureStdout();
EXPECT_TRUE(where == Cpp::Construct(scope, where, a)); // placement new
// Check for the value of x which should be at the start of the object.
EXPECT_TRUE(*(int*)where == 42);
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 == 42);
               ^

@aaronj0 aaronj0 force-pushed the structor-api branch 2 times, most recently from 0b3d49d to b6d01ac Compare June 2, 2025 13:54
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 22. Check the log or trigger a new build to see more.

else if (m_Kind == kConstructorCall && result && !args.m_Args) {
InvokeConstructor(result, /*nary=*/1UL, args, false);
return;
}
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 'else' after 'return' [llvm-else-after-return]

Suggested change
}
if (m_Kind == kConstructorCall && result && !args.m_Args) {
InvokeConstructor(result, /*nary=*/1UL, args, false);
return;
}

#ifndef NDEBUG
assert(AreArgumentsValid(result, args, nullptr, nary) && "Invalid args!");
ReportInvokeStart(result, args, nullptr);
#endif // NDEBUG
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 access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

    m_ConstructorCall(result, nary, args.m_ArgSize, args.m_Args, (int)is_arena);
    ^

@@ -768,19 +798,24 @@ enum : long int {
CPPINTEROP_API std::vector<long int> GetDimensions(TCppType_t type);

/// Allocates memory for a given class.
CPPINTEROP_API TCppObject_t Allocate(TCppScope_t scope);
/// \c count is used to indicate the number of objects to allocate for.
CPPINTEROP_API TCppObject_t Allocate(TCppScope_t scope,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum '(unnamed enum at /github/workspace/include/CppInterOp/CppInterOp.h:802:1)' uses a larger base type ('long', size: 8 bytes) than necessary for its value set, consider using 'std::int8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum : long int {
^

Copy link
Contributor

Choose a reason for hiding this comment

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

We should adjust the types to match the signature types to match the generated code types and disable the clang-tidy complaints if we have to.

const auto* CD = cast<CXXConstructorDecl>((const Decl*)m_FD);
if (CD->getMinRequiredArguments() != 0 && nary > 1) {
assert(0 &&
"Cannot pass initialization parameters to array new construction");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:39:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <cstddef>
+ #if CLANG_VERSION_MAJOR >= 19

}

// Standard branch:
// (*(ClassName**)ret) = (obj) ? new (*(ClassName**)ret) ClassName(args...)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto *CD' can be declared as 'const auto *CD' [readability-qualified-auto]

Suggested change
// (*(ClassName**)ret) = (obj) ? new (*(ClassName**)ret) ClassName(args...)
const auto* CD = dyn_cast<CXXConstructorDecl>(FD);

@@ -2831,6 +2904,13 @@ CPPINTEROP_API JitCall MakeFunctionCallable(TInterp_t I,
return {};
}

if (const auto* Ctor = dyn_cast<CXXConstructorDecl>(D)) {
if (auto Wrapper = make_wrapper(*interp, cast<FunctionDecl>(D)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::make_pair" is directly included [misc-include-cleaner]

    gDtorWrapperStore.insert(make_pair(D, F));
                             ^

}

// FIXME: Add optional arguments to the operator new.
TCppObject_t Construct(compat::Interpreter& interp, TCppScope_t scope,
void* arena /*=nullptr*/) {
void* arena /*=nullptr*/, TCppIndex_t count /*=1UL*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage]

  return PI->getNameAsString();
         ^
Additional context

lib/CppInterOp/CppInterOp.cpp:3613: 'PI' initialized to a null pointer value

  clang::ParmVarDecl* PI = nullptr;
  ^

lib/CppInterOp/CppInterOp.cpp:3615: Assuming null pointer is passed into cast

  if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
                 ^

lib/CppInterOp/CppInterOp.cpp:3615: 'FD' is null

  if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
            ^

lib/CppInterOp/CppInterOp.cpp:3615: Taking false branch

  if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
  ^

lib/CppInterOp/CppInterOp.cpp:3617: Assuming null pointer is passed into cast

  else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
                      ^

lib/CppInterOp/CppInterOp.cpp:3617: 'FD' is null

  else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
                 ^

lib/CppInterOp/CppInterOp.cpp:3617: Taking false branch

  else if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
       ^

lib/CppInterOp/CppInterOp.cpp:3620: Called C++ object pointer is null

  return PI->getNameAsString();
         ^

return Construct(getInterp(), scope, arena);
TCppObject_t Construct(TCppScope_t scope, void* arena /*=nullptr*/,
TCppIndex_t count /*=0UL*/) {
return Construct(getInterp(), scope, arena, count);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity' [clang-analyzer-optin.core.EnumCastOutOfRange]

  return (OperatorArity)~0U;
         ^
Additional context

include/CppInterOp/CppInterOp.h:93: enum declared here

enum OperatorArity : unsigned char { kUnary = 1, kBinary, kBoth };
     ^

lib/CppInterOp/CppInterOp.cpp:3625: Assuming 'D' is not a 'CastReturnType'

  if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
                 ^

lib/CppInterOp/CppInterOp.cpp:3625: 'FD' is null

  if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
            ^

lib/CppInterOp/CppInterOp.cpp:3625: Taking false branch

  if (auto* FD = llvm::dyn_cast<FunctionDecl>(D)) {
  ^

lib/CppInterOp/CppInterOp.cpp:3643: The value '4294967295' provided to the cast expression is not in the valid range of values for 'OperatorArity'

  return (OperatorArity)~0U;
         ^

@@ -1883,7 +1883,7 @@ TEST(FunctionReflectionTest, Construct) {
testing::internal::CaptureStdout();
auto* I = clang_createInterpreterFromRawPtr(Cpp::GetInterpreter());
auto scope_c = make_scope(static_cast<clang::Decl*>(scope), I);
auto object_c = clang_construct(scope_c, nullptr);
auto object_c = clang_construct(scope_c, nullptr, 1UL);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto object_c' can be declared as 'auto *object_c' [llvm-qualified-auto]

Suggested change
auto object_c = clang_construct(scope_c, nullptr, 1UL);
auto *object_c = clang_construct(scope_c, nullptr, 1UL);

// Check for the value of x which should be at the start of the object.
EXPECT_TRUE(*(int*)where == 42);
// Check for the value of x in the second object
int* obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
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]

  int* obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
                                    ^

@aaronj0 aaronj0 marked this pull request as ready for review June 2, 2025 13:56
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 20. Check the log or trigger a new build to see more.

assert(AreArgumentsValid(result, args, nullptr, nary) && "Invalid args!");
ReportInvokeStart(result, args, nullptr);
#endif // NDEBUG
m_ConstructorCall(result, nary, args.m_ArgSize, args.m_Args, (int)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: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

    m_ConstructorCall(result, nary, args.m_ArgSize, args.m_Args, (int)is_arena);
    ^

@@ -132,7 +132,8 @@ static clang::Sema& getSema() { return getInterp().getCI()->getSema(); }
static clang::ASTContext& getASTContext() { return getSema().getASTContext(); }

#define DEBUG_TYPE "jitcall"
bool JitCall::AreArgumentsValid(void* result, ArgList args, void* self) const {
bool JitCall::AreArgumentsValid(void* result, ArgList args, void* self,
size_t nary) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:39:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <cstddef>
+ #if CLANG_VERSION_MAJOR >= 19

"Number of objects to construct/destruct should be atleast 1");

if (Cpp::IsConstructor(m_FD)) {
const auto* CD = cast<CXXConstructorDecl>((const Decl*)m_FD);
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]

    const auto* CD = cast<CXXConstructorDecl>((const Decl*)m_FD);
                                              ^

{
std::ostringstream typedefbuf;
std::ostringstream callbuf;
//
// Write the return value assignment part.
//
indent(callbuf, indent_level);
auto* CD = dyn_cast<CXXConstructorDecl>(FD);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto *CD' can be declared as 'const auto *CD' [readability-qualified-auto]

Suggested change
auto* CD = dyn_cast<CXXConstructorDecl>(FD);
const auto* CD = dyn_cast<CXXConstructorDecl>(FD);

TCppObject_t Allocate(TCppScope_t scope) {
return (TCppObject_t)::operator new(Cpp::SizeOf(scope));
TCppObject_t Allocate(TCppScope_t scope, TCppIndex_t count) {
return (TCppObject_t)::operator new(Cpp::SizeOf(scope) * count);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "operator new" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:39:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <new>
+ #if CLANG_VERSION_MAJOR >= 19

::operator delete(address);
void Deallocate(TCppScope_t scope, TCppObject_t address, TCppIndex_t count) {
size_t bytes = Cpp::SizeOf(scope) * count;
::operator delete(address, bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "operator delete" is directly included [misc-include-cleaner]

  ::operator delete(address, bytes);
    ^

@@ -3613,33 +3694,36 @@ TCppObject_t Construct(compat::Interpreter& interp, TCppScope_t scope,
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.InvokeConstructor(&arena, count, {},
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(&arena, count, {},
                           ^

return arena;
}

void* obj = nullptr;
JC.Invoke(&obj);
JC.InvokeConstructor(&obj, count, {}, false);
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(&obj, count, {}, false);
                         ^

// Check for the value of x which should be at the start of the object.
EXPECT_TRUE(*(int*)where == 42);
// Check for the value of x in the second object
int* obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
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]

  int* obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
             ^

EXPECT_TRUE(*obj == 42);

// Check for the value of x in the last object
obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
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]

  obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
                               ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 15. Check the log or trigger a new build to see more.

else if (m_Kind == kConstructorCall && result && !args.m_Args) {
InvokeConstructor(result, /*nary=*/1UL, args, self);
return;
}
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 'else' after 'return' [llvm-else-after-return]

Suggested change
}
if (m_Kind == kConstructorCall && result && !args.m_Args) {
InvokeConstructor(result, /*nary=*/1UL, args, self);
return;
}

assert(AreArgumentsValid(result, args, nullptr, nary) && "Invalid args!");
ReportInvokeStart(result, args, nullptr);
#endif // NDEBUG
m_ConstructorCall(result, nary, args.m_ArgSize, args.m_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: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

    m_ConstructorCall(result, nary, args.m_ArgSize, args.m_Args, is_arena);
    ^

@@ -3613,33 +3694,36 @@ TCppObject_t Construct(compat::Interpreter& interp, TCppScope_t scope,
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.InvokeConstructor(&arena, count, {},
(void*)~0); // Tell Invoke to use placement new.
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]

                           (void*)~0); // Tell Invoke to use placement new.
                           ^

@@ -3613,33 +3694,36 @@
auto* const Ctor = GetDefaultConstructor(interp, Class);
if (JitCall JC = MakeFunctionCallable(&interp, Ctor)) {
if (arena) {
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
JC.InvokeConstructor(&arena, count, {},
(void*)~0); // Tell Invoke to use placement new.
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

                           (void*)~0); // Tell Invoke to use placement new.
                           ^

return arena;
}

void* obj = nullptr;
JC.Invoke(&obj);
JC.InvokeConstructor(&obj, count, {}, 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: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

    JC.InvokeConstructor(&obj, count, {}, nullptr);
                         ^


// Check for the value of x in the last object
obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
Cpp::SizeOf(scope) * 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
Cpp::SizeOf(scope) * 4);
(Cpp::SizeOf(scope) * 4));

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


// verify the array of objects has been constructed
int* obj = reinterpret_cast<int*>(reinterpret_cast<char*>(where) +
Cpp::SizeOf(scope) * 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
Cpp::SizeOf(scope) * 4);
(Cpp::SizeOf(scope) * 4));

testing::internal::CaptureStdout();

// destruct the rest
auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto new_head' can be declared as 'auto *new_head' [llvm-qualified-auto]

Suggested change
auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
auto *new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +

testing::internal::CaptureStdout();

// destruct the rest
auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
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]

  auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
                                          ^

testing::internal::CaptureStdout();

// destruct the rest
auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
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]

  auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
                  ^


// destruct the rest
auto new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
Cpp::SizeOf(scope) * 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
Cpp::SizeOf(scope) * 3);
(Cpp::SizeOf(scope) * 3));

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