Copilot commented on code in PR #409:
URL: https://github.com/apache/fluss-rust/pull/409#discussion_r2870177148
##########
bindings/cpp/src/lib.rs:
##########
@@ -630,13 +626,34 @@ fn err_from_core_error(e: &fcore::error::Error) ->
ffi::FfiResult {
}
}
+fn ok_ptr(ptr: i64) -> ffi::FfiPtrResult {
+ ffi::FfiPtrResult {
+ result: ok_result(),
+ ptr,
+ }
+}
+
Review Comment:
Helper `ok_ptr(ptr: i64)` combined with multiple `ok_ptr(ptr as i64)` sites
relies on pointer->`i64` casts. If `FfiPtrResult.ptr` is switched to
`usize`/`u64`, update these helpers to avoid lossy casts and keep the pointer
round-trip well-defined across platforms.
##########
bindings/cpp/src/lib.rs:
##########
@@ -247,6 +247,11 @@ mod ffi {
server_nodes: Vec<FfiServerNode>,
}
+ struct FfiPtrResult {
+ result: FfiResult,
+ ptr: i64,
Review Comment:
`FfiPtrResult.ptr` is defined as `i64` and populated via `ptr as i64`.
Casting raw pointers through a signed 64-bit integer can truncate on platforms
where `usize` > 64 bits, and can also mis-handle addresses above `i64::MAX` on
64-bit systems. Prefer using `usize` (or `u64`) for the pointer field in the
CXX bridge and converting via `usize`/`uintptr_t` to avoid
signedness/truncation pitfalls.
```suggestion
ptr: usize,
```
##########
bindings/cpp/src/ffi_converter.hpp:
##########
@@ -37,6 +37,11 @@ inline Result from_ffi_result(const ffi::FfiResult&
ffi_result) {
return Result{ffi_result.error_code,
std::string(ffi_result.error_message)};
}
+template <typename T>
+inline T* ptr_from_ffi(const ffi::FfiPtrResult& r) {
+ return reinterpret_cast<T*>(r.ptr);
+}
Review Comment:
`ptr_from_ffi` currently does `reinterpret_cast<T*>(r.ptr)` where `ptr` is
an `i64`. Converting from a signed integer to a pointer is
implementation-defined and can behave unexpectedly for values with the high bit
set. Prefer making the bridge field an unsigned pointer-sized integer
(`usize`/`uintptr_t`) and casting via `static_cast<uintptr_t>(...)` before
`reinterpret_cast`.
##########
bindings/cpp/src/connection.cpp:
##########
@@ -47,15 +47,13 @@ Connection& Connection::operator=(Connection&& other)
noexcept {
}
Result Connection::Create(const Configuration& config, Connection& out) {
- try {
- auto ffi_config = utils::to_ffi_config(config);
- out.conn_ = ffi::new_connection(ffi_config);
- return utils::make_ok();
- } catch (const rust::Error& e) {
- return utils::make_client_error(e.what());
- } catch (const std::exception& e) {
- return utils::make_client_error(e.what());
+ auto ffi_config = utils::to_ffi_config(config);
+ auto ffi_result = ffi::new_connection(ffi_config);
+ auto result = utils::from_ffi_result(ffi_result.result);
+ if (result.Ok()) {
+ out.conn_ = utils::ptr_from_ffi<ffi::Connection>(ffi_result);
}
+ return result;
Review Comment:
This change is intended to preserve server-side error codes for
pointer-returning methods. There are existing C++ integration tests (e.g. SASL
auth failure cases) that currently have TODOs because `Connection::Create` used
to return `CLIENT_ERROR` due to `Result<*mut T>` conversion. Please update/add
tests to assert the expected server error codes now propagate (e.g.
`AUTHENTICATE_EXCEPTION`) so the regression that motivated #383 stays covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
