Page MenuHomePhabricator

[CommCoreModule] Fill out `sessionSignature(deviceID) => toSign` boilerplate in `Client`
ClosedPublic

Authored by atul on Feb 8 2022, 1:35 PM.
Tags
None
Referenced Files
F3301801: D3143.id9484.diff
Mon, Nov 18, 4:19 AM
F3301797: D3143.id9416.diff
Mon, Nov 18, 4:15 AM
F3301644: D3143.diff
Mon, Nov 18, 2:47 AM
Unknown Object (File)
Sat, Nov 2, 4:38 AM
Unknown Object (File)
Fri, Oct 25, 12:01 AM
Unknown Object (File)
Fri, Oct 25, 12:01 AM
Unknown Object (File)
Fri, Oct 25, 12:01 AM
Unknown Object (File)
Fri, Oct 25, 12:00 AM

Details

Summary

So we can hit SessionSignature(...) endpoint

Test Plan

Hit tunnelbroker with request and see if we get a response

Diff Detail

Repository
rCOMM Comm
Branch
landfeb8 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 8 2022, 1:38 PM
Harbormaster failed remote builds in B6651: Diff 9416!
atul requested review of this revision.Feb 8 2022, 1:44 PM
This revision is now accepted and ready to land.Feb 8 2022, 1:47 PM
This revision now requires review to proceed.Feb 8 2022, 1:47 PM
ashoat added a reviewer: jim.
ashoat added inline comments.
native/cpp/CommonCpp/grpc/Client.cpp
138

So what's going on here... NULL is a c-str and that get initialized into an std::string? Is this the best pattern?

This revision is now accepted and ready to land.Feb 8 2022, 9:40 PM
native/cpp/CommonCpp/grpc/Client.cpp
138

NULL == nullptr since C++ 11 so the function would return nullptr

At the callsite we'll check the truthiness of the return value

138

Can be more explicit and return nullptr?

native/cpp/CommonCpp/grpc/Client.cpp
138

Just kidding... Looks like the right thing to do is

return std::string{};

and check for empty string

if (blah.empty()) {
  return;
}
native/cpp/CommonCpp/grpc/Client.cpp
138

Alternatively I guess could return a std::optional<std::string>> or tuple of (grpc::Status, std::optional<std::string>) or something

replace NULL with empty string

This revision was landed with ongoing or failed builds.Feb 9 2022, 10:34 AM
This revision was automatically updated to reflect the committed changes.