Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider reordering arguments for Async* member functions. #1543

Closed
coryan opened this issue Nov 28, 2018 · 4 comments · Fixed by #1763
Closed

Consider reordering arguments for Async* member functions. #1543

coryan opened this issue Nov 28, 2018 · 4 comments · Fixed by #1763
Assignees
Labels
api: bigtable Issues related to the Bigtable API.

Comments

@coryan
Copy link
Contributor

coryan commented Nov 28, 2018

In the original design document I recommended that a function like:

type1 Function(type2 a, type3 b);

was converted to an asynchronous version by (amongst other changes) appending two arguments:

template <typename Functor>
shared_ptr<AsyncOperation> AsyncFunction(
    type2 a, type3 b,
    CompletionQueue& cq, Functor&& f);

That works poorly for functions that have default parameters:

type1 WithDefault(type2 a, type3 b = 0);

template <typename Functor>
shared_ptr<AsyncOperation> AsyncWithDefault(
  type2 a, type3 b = 0,      // ERROR, cannot have default in the middle.
  CompletionQueue& cq,
  Functor&& f);

or with variadic arguments:

template <typename Args>
type1 WithVariadic(type2 a, Args&&... args);

template <typename Functor, typename Args>
shared_ptr<AsyncOperation> AsyncWithVariadic(
  type2 a, Args&&... args, // ERROR, cannot variadic args in the middle
  CompletionQueue& cq,
  Functor&& f);

I propose we change the order of the arguments to have the completion queue and the functor at the beginning:

type1 WithDefault(type2 a, type3 b = 0);

template <typename Functor>
shared_ptr<AsyncOperation> AsyncWithDefault(
  CompletionQueue& cq, Functor&& f,
  type2 a, type3 b = 0                                // OK.
  );

template <typename Args>
type1 WithVariadic(type2 a, Args&&... args);

template <typename Functor, typename Args>
shared_ptr<AsyncOperation> AsyncWithVariadic(
  CompletionQueue& cq, Functor&& f,
  type2 a, Args&&... args,                          // OK.
  );

The downside is that calling the function with a lambda at the end is more readable (most of the time):

auto op = AsyncWithDefault(cq, [](CompletionQueue& cq, type1 result, Status& status) {
  // Do callback stuff here.
  // it can get long....
  },
  arg1, arg2); // these may be hard to find and read in real code.
@coryan coryan added the api: bigtable Issues related to the Bigtable API. label Nov 28, 2018
@coryan coryan added this to the Cloud Bigtable: Async APIs milestone Nov 28, 2018
@coryan
Copy link
Contributor Author

coryan commented Nov 28, 2018

@dopiera @houglum @roopak-qlogic @manish-qlogic @deepankarsharma PTAL.

Comments (or simply up/down votes) are welcome.

@coryan
Copy link
Contributor Author

coryan commented Dec 1, 2018

If there are no opinions by 2018-12-07 I am going to consider this proposal accepted.

@dopiera
Copy link
Contributor

dopiera commented Dec 3, 2018

upvote!

@houglum
Copy link
Contributor

houglum commented Dec 3, 2018

This seems reasonable, LGTM.

W.r.t. readability and passing a lambda in the middle args vs. at the end: As an alternative to creating it inline in the list of args, I'd imagine you could declare the lambda outside of the arg list, then pass its name as an argument (via std::move, if you don't want to copy it or have the calling scope keep ownership):

auto some_callable = ...;
auto op = AsyncWithFoo(cq, std::move(some_callable), arg1, arg2);

@coryan coryan self-assigned this Jan 3, 2019
coryan added a commit to coryan/google-cloud-cpp that referenced this issue Jan 3, 2019
As described in googleapis#1543, it seems better to move the arguments for a
`Async*()` function to be

```C++
AsyncBlah(CompletionQueue& cq, Functor&& callback, Foo)
```

This fixes googleapis#1543.
coryan added a commit that referenced this issue Jan 4, 2019
As described in #1543, it seems better to move the arguments for a
`Async*()` function to be

```C++
AsyncBlah(CompletionQueue& cq, Functor&& callback, Foo)
```

This fixes #1543.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants