Page MenuHomePhabricator
Differential D4936

[native] restructure functions to be exposed across FFI boundary
ClosedPublic
Actions

Authored by varun on Aug 23 2022, 10:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 30, 6:03 AM
Unknown Object (File)
Sun, Jun 30, 5:47 AM
Unknown Object (File)
Sat, Jun 29, 3:12 PM
Unknown Object (File)
Thu, Jun 27, 1:14 AM
Unknown Object (File)
Thu, Jun 27, 12:41 AM
Unknown Object (File)
Tue, Jun 18, 9:38 AM
Unknown Object (File)
Tue, Jun 18, 9:38 AM
Unknown Object (File)
Tue, Jun 18, 9:31 AM
View All 47 Files
Subscribers
• abosh
ashoat

Details

Reviewers
tomek
• karol
atul
Commits
rCOMM26ec48a1d823: [native] restructure functions to be exposed across FFI boundary
Summary

CXX doesn't have as much support for methods as I thought, so I'm changing the methods to normal functions that take an additional client param. We use a Box pointer because opaque Rust types can't be passed directly. I also removed all opaque Rust types from the return values of these functions.

Depends on D4935

Test Plan

cargo build, this is just a slight refactor even though it looks big

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun created this revision.Aug 23 2022, 10:44 PM
Herald added subscribers: • abosh, ashoat. · View Herald TranscriptAug 23 2022, 10:44 PM
varun added a child revision: D4937: [native] more blocking functions.Aug 23 2022, 10:45 PM
Harbormaster completed remote builds in B11565: Diff 15901.Aug 23 2022, 10:54 PM
varun requested review of this revision.Aug 23 2022, 10:54 PM
tomek requested changes to this revision.Aug 24 2022, 3:08 AM

Could you explain a bit what do you mean by

CXX doesn't have as much support for methods as I thought

?
It seems like cxx supports methods https://cxx.rs/extern-rust.html#methods

This revision now requires changes to proceed.Aug 24 2022, 3:08 AM
varun requested review of this revision.Aug 24 2022, 11:57 AM
In D4936#142927, @tomek wrote:

Could you explain a bit what do you mean by

CXX doesn't have as much support for methods as I thought

?
It seems like cxx supports methods https://cxx.rs/extern-rust.html#methods

Sorry, methods are supported. The issue is that it doesn't recognize the new() constructor in the impl block. I wasn't able to add this function to the ffi module

tomek accepted this revision.Aug 25 2022, 1:37 AM
In D4936#143216, @varun wrote:
In D4936#142927, @tomek wrote:

Could you explain a bit what do you mean by

CXX doesn't have as much support for methods as I thought

?
It seems like cxx supports methods https://cxx.rs/extern-rust.html#methods

Sorry, methods are supported. The issue is that it doesn't recognize the new() constructor in the impl block. I wasn't able to add this function to the ffi module

Thanks for the explanation! It's surprising that cxx doesn't support that, but spending too much time on it doesn't make sense when we have a working solution.

This revision is now accepted and ready to land.Aug 25 2022, 1:37 AM
Closed by commit rCOMM26ec48a1d823: [native] restructure functions to be exposed across FFI boundary (authored by varun). · Explain WhyAug 25 2022, 3:57 AM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM26ec48a1d823: [native] restructure functions to be exposed across FFI boundary.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
grpc/
grpc_client/
src/
399 lines

Diff 15968

View Options

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs

Loading...