Paths

Table of Contentst

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

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

Authored by varun on Aug 23 2022, 10:43 PM.

Details

Reviewers

tomek
• karol
atul

Commits

rCOMMb055e9dcb91f: [native] change params and types to satisfy cxx requirements

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Aug 23 2022, 10:43 PM

Herald added subscribers: • abosh, ashoat. · View Herald TranscriptAug 23 2022, 10:43 PM

varun added a child revision: D4935: [native] make output a static library.Aug 23 2022, 10:47 PM

Harbormaster completed remote builds in B11563: Diff 15899.Aug 23 2022, 10:52 PM

varun requested review of this revision.Aug 23 2022, 10:52 PM

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	`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

remove unnecessary cast

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	we don't! good catch

Harbormaster completed remote builds in B11602: Diff 15950.Aug 24 2022, 8:25 PM

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

https://linear.app/comm/issue/ENG-1702/improve-grpc-client

Closed by commit rCOMMb055e9dcb91f: [native] change params and types to satisfy cxx requirements (authored by varun). · Explain WhyAug 25 2022, 3:47 AM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMMb055e9dcb91f: [native] change params and types to satisfy cxx requirements.

Revision Contents
Changeset List

Path

Size

native/

cpp/

CommonCpp/

grpc/

grpc_client/

src/

lib.rs

29 lines

Diff 15899

View Options

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

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines	async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult;		let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult;		const [, canRequestPermission] = permissionPromisesResult;

if (!hasPermission && canRequestPermission) {		if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => {		const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt		// We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears		// doesn't appear immediately but after logged-out modal disappears
await sleep(10);		await sleep(10);
await this.requestAndroidNotificationsPermission();		return await this.requestAndroidNotificationsPermission();
		marcinUnsubmitted Not Done Inline Actions We definitely should return something from this lambda since otherwise it is useless to assign it to variable. When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android. If we find that we don't have permissions then just calling `await this.requestAndroidNotificationsPermission();` would result in a prompt asking for notifications permissions. However if the user grants those permissions then `hasPermissions` is still falsy (since promise returned nothing), so `deviceToken` will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render. This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state. marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})();		})();
hasPermission = permissionResponse;		hasPermission = permissionResponse;
}		}

if (!hasPermission) {		if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);		this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return;		return;
}		}
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines

[native] change params and types to satisfy cxx requirementsClosedPublicActions