enhance: Handle rust error in c++ #38113
Conversation
mergify
bot
added
dco-passed
|
@sunby Please associate the related issue to the body of your Pull Request. (eg. “issue: #”) |
|
@sunby E2e jenkins job failed, comment |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #38113 +/- ##
==========================================
- Coverage 82.78% 80.88% -1.91%
==========================================
Files 1084 1373 +289
Lines 166888 193165 +26277
==========================================
+ Hits 138157 156240 +18083
- Misses 23209 31395 +8186
- Partials 5522 5530 +8
|
|
@sunby E2e jenkins job failed, comment |
sunby
force-pushed
the
return_rust_error_msg
branch
from
b397df9 to
c64714e
Compare
| explicit RustResultWrapper(RustResult result) : result_(result) { | ||
| } | ||
|
|
||
| RustResultWrapper(RustResultWrapper&& other) noexcept { |
There was a problem hiding this comment.
It seems that RustResultWrapper is not a safe type.
The move construction operation on it make a double free.
I prefer to wrap those ffi type such as RustResult with unique_ptr to make it safe.
| #[no_mangle] | ||
| pub extern "C" fn free_rust_array(array: RustArray) { | ||
| let RustArray { array, len, cap } = array; | ||
| unsafe { | ||
| Vec::from_raw_parts(array, len, cap); | ||
| } | ||
| } | ||
|
|
||
| #[repr(C)] |
There was a problem hiding this comment.
does the ffi type satisfy the rfc https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md?
Please add some unittest to make sure that it's ok at current compiler.
There was a problem hiding this comment.
does the
ffitype satisfy the rfc https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md? Please add some unittest to make sure that it's ok at current compiler.
as cbindgen doc says cbindgen also supports using repr(C)/repr(u8) on non-C-like enums (enums with fields). This gives a C-compatible tagged union layout, as [defined by this RFC 2195](https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md). repr(C) will give a simpler layout that is perhaps more intuitive, while repr(u8) will produce a more compact layout.
this enum should be supported. But I will also add a ut to verify it.
| #[no_mangle] | ||
| pub extern "C" fn free_rust_result(result: RustResult) { | ||
| match result.value { | ||
| Value::RustArray(array) => { |
There was a problem hiding this comment.
In my view, the pointer type should also be freed.
There was a problem hiding this comment.
In my view, the pointer type should also be freed.
Ptr type is catched by c++ class and will be freed when dtor is called for now. So we don't free ptr here.
|
@sunby go-sdk check failed, comment |
|
@sunby cpp-unit-test check failed, comment |
|
@sunby E2e jenkins job failed, comment |
Signed-off-by: sunby <[email protected]>
Signed-off-by: sunby <[email protected]>
Signed-off-by: sunby <[email protected]>
Signed-off-by: sunby <[email protected]>
sunby
force-pushed
the
return_rust_error_msg
branch
from
fbd0726 to
599c31a
Compare
|
@sunby cpp-unit-test check failed, comment |
Signed-off-by: sunby <[email protected]>
|
@sunby cpp-unit-test check failed, comment |
Signed-off-by: sunby <[email protected]>
|
@sunby go-sdk check failed, comment |
|
@sunby cpp-unit-test check failed, comment |
Signed-off-by: sunby <[email protected]>
|
@sunby cpp-unit-test check failed, comment |
Signed-off-by: sunby <[email protected]>
|
@sunby go-sdk check failed, comment |
|
rerun go-sdk |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia, sunby The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
#37930