Page MenuHomePhabricator

[native] change params and types to satisfy cxx requirements
ClosedPublic

Authored by varun on Aug 23 2022, 10:43 PM.
Tags
None
Referenced Files
F3151179: D4934.diff
Tue, Nov 5, 2:06 AM
Unknown Object (File)
Oct 6 2024, 2:12 AM
Unknown Object (File)
Oct 6 2024, 2:12 AM
Unknown Object (File)
Oct 6 2024, 2:12 AM
Unknown Object (File)
Oct 6 2024, 2:12 AM
Unknown Object (File)
Oct 6 2024, 2:09 AM
Unknown Object (File)
Sep 20 2024, 7:35 PM
Unknown Object (File)
Sep 20 2024, 5:47 PM
Subscribers

Details

Summary

Changed auth_type to an i32 to make it easier to pass across the FFI boundary. Also changed a return type to a String for the same reason.

Depends on D4933

Test Plan

cargo build

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Aug 24 2022, 2:57 AM

Changed auth_type to an i32 to make it easier to pass across the FFI boundary.

Is it a good practice to use i32 instead of enum? How hard is it to pass an enum?

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
88 ↗(On Diff #15899)

auth_type is already i32 here, so why do we need to cast it?

This revision now requires changes to proceed.Aug 24 2022, 2:57 AM
In D4934#142918, @tomek wrote:

Changed auth_type to an i32 to make it easier to pass across the FFI boundary.

Is it a good practice to use i32 instead of enum? How hard is it to pass an enum?

It's not a great practice but it's hard to pass an enum. If the enum is defined in the ffi module, the compiler complains:

non-primitive cast: `ffi::AuthType` as `i32`
an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

And if I try to define it in the Rust library, the compiler complains about passing opaque Rust types by value.

I can explore a better solution and revisit this later, if that works for you.

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
88 ↗(On Diff #15899)

we don't! good catch

In D4934#143266, @varun wrote:
In D4934#142918, @tomek wrote:

Changed auth_type to an i32 to make it easier to pass across the FFI boundary.

Is it a good practice to use i32 instead of enum? How hard is it to pass an enum?

It's not a great practice but it's hard to pass an enum. If the enum is defined in the ffi module, the compiler complains:

non-primitive cast: `ffi::AuthType` as `i32`
an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

And if I try to define it in the Rust library, the compiler complains about passing opaque Rust types by value.

I can explore a better solution and revisit this later, if that works for you.

Yeah, a small research sounds like a good idea. The safer we make this code, the less problems we will have in the future. A quick search gave me this https://rust-lang.github.io/unsafe-code-guidelines/layout/enums.html#repr-annotations-accepted-on-enums - not sure, but it might be helpful.

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
88 ↗(On Diff #15950)

We should be able to use a shorthand now

This revision is now accepted and ready to land.Aug 25 2022, 1:28 AM