Paths

Table of Contentst

[services] Tunnelbroker - Using `rust::notifications` namespace for calling Rust from C++
ClosedPublic
Actions

Authored by • max on Sep 29 2022, 7:38 AM.

Details

Reviewers

• jon
michal
marcin
tomek

Commits

rCOMM2a895ae97c6b: [services] Tunnelbroker - Using `rust::notifications` namespace for calling…

Summary

This diff introduces changes using the rust::notifications namespace for calling Rust notification functions from C++.

At the moment in Tunnelbroker, we are calling the Rust function without using namespaces. The Rust functions are exposed as global to C++. The better way is to use namespaces in Rust code to expose it to C++ codebase.

Related Linear task: ENG-1894

Test Plan

Service successfully built.
Tests passed.

Diff Detail

Repository

rCOMM Comm

Branch

rust-use-namespace

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Sep 29 2022, 7:38 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 29 2022, 7:38 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D5261: [services] Tunnelbroker - Renaming Rust library directory from `rust-notifications` to `rust-lib`.Sep 29 2022, 7:38 AM

• max edited the summary of this revision. (Show Details)Sep 29 2022, 7:42 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: michal, tomek.

• max published this revision for review.Sep 29 2022, 7:44 AM

• max added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
11–25	I didn't use a global namespace here now because of the adding a new namespace for the gRPC-related functions very soon and we need to distinguish them.

Harbormaster completed remote builds in B12528: Diff 17192.Sep 29 2022, 7:48 AM

@max please add @marcin as a reviewer for Rust / C++ diffs - he will add me as a blocking reviewer after the diff is accepted.

• jon added inline comments.Sep 29 2022, 9:49 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	I feel like typedefs in a source file are kind of an anti pattern. Or maybe I'm just used to rust's import syntax. not sure how @tomek feels

marcin added inline comments.Sep 30 2022, 1:19 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	I agree with @jon that typedef should be defined in headers file. If we want to use them in other places in the code they should be defined in header files. Otherwise if we just want those types to be used locally in a source file we may probably use `auto` instead.

tomek added inline comments.Sep 30 2022, 2:55 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	I'm not strictly against them - if they are local and increase readability, it's fine. An antipattern in C++, that should be avoided, is using `using`.

ashoat added inline comments.Sep 30 2022, 4:29 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	What's the downside of a `typedef` here?

• max marked 4 inline comments as done.Sep 30 2022, 5:11 AM

• max added inline comments.

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	I feel like typedefs in a source file are kind of an anti pattern. Or maybe I'm just used to rust's import syntax. not sure how @tomek feels In general, yes, but it depends. In this case, it's used to make the code more readable because there are a long "train" of the namespace.
173	I'm not strictly against them - if they are local and increase readability, it's fine. That's right - it's local, and it is used to increase the code readability instead of calling the long namespace. An antipattern in C++, that should be avoided, is using `using`. Agree! We can't make a namespace local but the typedef could.
173	What's the downside of a `typedef` here? Personally, I don't see any downsides here, but the pros are: Code readability is increased; It's local; It does allocate nothing because it's just an alias. We have a really long name here and it's unreadable without some aliasing.

In D5263#155118, @tomek wrote:

@max please add @marcin as a reviewer for Rust / C++ diffs - he will add me as a blocking reviewer after the diff is accepted.

Ok, I'll do that ;)

• max added inline comments.Sep 30 2022, 5:14 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	Otherwise if we just want those types to be used locally in a source file we may probably use `auto` instead. `Typdef` and `Auto` have different purposes and are not interchangeable: typdef is a type alias, but `auto` is a type auto assigning.

marcin accepted this revision.Sep 30 2022, 6:30 AM

marcin added a reviewer: tomek.

An antipattern in C++, that should be avoided, is using using.

Agreed. otherwise LGTM.

I'm not strongly opinionated about the typedef, because I don't think there's a strictly better way to do it.

• jon added inline comments.Sep 30 2022, 10:06 AM

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
173	What's the downside of a typedef here? It's acting as an alias here, so instead of using the fully qualified `rust::notifications::apnsReturnStatus`, just `apnsReturnStatus` can be used. A potential downside is just slightly increasing the things defined at this scope. But I don't think its worse than having to use the full path in this one case. It's somewhat similar to the `using <namespace>` anti pattern, but that's usually a bigger problem because you could be introducing 1,000's of items at a scope.

I'm not a big fan of typedef too, but using fully qualified names here looks very ugly (looks like a Flutter-way ;) ).
If you guys think it looks ok and readable I'll remove the typdef here. I do not insist on it, it's just some prettifying.

I still don't see any downsides to the typedef

I think we should keep the typedef here - fully qualified names would reduce the readability,

Just to add something to the discussion, C++ guideline seems to prefer using over typedef for aliases https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#t43-prefer-using-over-typedef-for-defining-aliases. I think it is relevant mostly when aliasing function pointer types.

This revision is now accepted and ready to land.Oct 3 2022, 2:27 AM

Rebased on master changes.

Harbormaster completed remote builds in B12579: Diff 17281.Oct 3 2022, 2:51 AM

Closed by commit rCOMM2a895ae97c6b: [services] Tunnelbroker - Using `rust::notifications` namespace for calling… (authored by • max). · Explain WhyOct 3 2022, 2:57 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM2a895ae97c6b: [services] Tunnelbroker - Using `rust::notifications` namespace for calling….