Improve Cpp::Construct pointer logic for memory arena flags #650
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
lib/CppInterOp/CppInterOp.cpp
Outdated
| return nullptr; | ||
| // invoke the constructor (placement/heap) in one shot | ||
| // flag is non-null for placement new, null for normal new | ||
| void* flag = arena ? reinterpret_cast<void*>(1) : nullptr; |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
void* flag = arena ? reinterpret_cast<void*>(1) : nullptr;
^
lib/CppInterOp/CppInterOp.cpp
Outdated
| // flag is non-null for placement new, null for normal new | ||
| void* flag = arena ? reinterpret_cast<void*>(1) : nullptr; | ||
| void* result = arena; | ||
| JC.InvokeConstructor(&result, count, {}, flag); |
There was a problem hiding this comment.
warning: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
JC.InvokeConstructor(&result, count, {}, flag);
^
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
==========================================
+ Coverage 78.79% 78.80% +0.01%
==========================================
Files 9 9
Lines 3852 3854 +2
==========================================
+ Hits 3035 3037 +2
Misses 817 817
🚀 New features to boost your workflow:
|
|
@aaronj0 Ignore the failing Emscripten job. It wasn't anything you did. It happened on one of my PRs. The Emscripten static library for llvm 19 is a little demanding for the Ubuntu Github runners. If you rerun the job it should pass. |
aaronj0
force-pushed
the
improve-cpp-construct
branch
from
92a2afa to
6d22d46
Compare
| testing::internal::CaptureStdout(); | ||
| where = Cpp::Allocate(scope); | ||
| EXPECT_TRUE(where == Cpp::Construct(SubDecls[2], where)); | ||
| EXPECT_TRUE(*(int *)where == 12345); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
EXPECT_TRUE(*(int *)where == 12345);
^
aaronj0
force-pushed
the
improve-cpp-construct
branch
2 times, most recently
from
7f632d3 to
f4c269a
Compare
| testing::internal::CaptureStdout(); | ||
| where = Cpp::Allocate(scope); | ||
| EXPECT_TRUE(where == Cpp::Construct(SubDecls[3], where)); | ||
| EXPECT_TRUE(*(int*)where == 12345); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
EXPECT_TRUE(*(int*)where == 12345);
^
| return nullptr; | ||
| // 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; |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
void* is_arena = arena ? reinterpret_cast<void*>(1) : nullptr;
^| // flag is non-null for placement new, null for normal new | ||
| void* is_arena = arena ? reinterpret_cast<void*>(1) : nullptr; | ||
| void* result = arena; | ||
| JC.InvokeConstructor(&result, count, /*args=*/{}, is_arena); |
There was a problem hiding this comment.
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);
^
Also handle both ctors and class scopes
aaronj0
force-pushed
the
improve-cpp-construct
branch
from
dba9728 to
6d11f91
Compare
|
@mcbarton There is something wrong with the Emscripten build jobs, they have been running for over an hour and (seems to have) stopped progressing past |
|
I am going ahead and merging this PR since the test that was extended is not run on windows, nor emscripten builds with llvm < 20 (which cover the 3 jobs that are stalled), but the issue on the CI must be investigated |
Adds various upstreamed developments: compiler-research/CppInterOp#650 streamlines the logic in Cpp::Construct to be more generic. compiler-research/CppInterOp#643 by @hageboeck making CppInterOp work with new gtest target names Fixed wrapper name conflicts with ROOT that also uses `__cf`, allowing us to integrate JitCall incrementally and co-exist with CallFunc Fixes related to the cppyy forks Unrelated work on Emscripten and WebAssembly
Adds various upstreamed developments: compiler-research/CppInterOp#650 streamlines the logic in Cpp::Construct to be more generic. compiler-research/CppInterOp#643 by @hageboeck making CppInterOp work with new gtest target names Fixed wrapper name conflicts with ROOT that also uses `__cf`, allowing us to integrate JitCall incrementally and co-exist with CallFunc Fixes related to the cppyy forks Unrelated work on Emscripten and WebAssembly
Also handle both ctors and class scopes. Upstreamed from root-project/root#18546