Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318113002:
Update //docs/callback.md for OnceCallback and RepeatingCallback (Closed)
DescriptionUpdate //docs/callback.md for OnceCallback and RepeatingCallback
Committed: https://crrev.com/7c0c0cf17a2c77837916ecfe2f9d83712f69a7bc
Cr-Commit-Position: refs/heads/master@{#423110}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 34
Patch Set 3 : . #
Total comments: 4
Patch Set 4 : minor rephrase #Messages
Total messages: 17 (8 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tzik@chromium.org changed reviewers: + dcheng@chromium.org, yutak@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for cleaning up the documentation, it adds a lot of useful details. As mentioned, I think it'd be OK to land this with https://codereview.chromium.org/2042223002/: I just wanted to review that CL rebased on top of the documentation move, to make the review easier. Just a few thoughts (if you prefer to keep it separate, I think that's fine, but it might be nice to land it as one logical unit) https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode24 docs/callback.md:24: `OnceCallback<>` is created by `BindOnce()` as a restricted variant of `OnceCallback<>` is created by `BindOnce()`. This is a callback variant that is a move-only type and can be run only once. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode26 docs/callback.md:26: movable types better as its bound parameter, and has clearer lifetime. Thus, Maybe clarify what 'better' means here. Perhaps omit the "and has clearer lifetime" from this sentence as well: we can mention this in the following sentence. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode27 docs/callback.md:27: thread hopping and result handning of an asynchronous operation are a good fit Typo: handling. But I would change this to indicate that we should default to OnceCallback unless there's good reason not to: This should be the preferred callback type: since the lifetime of the callback is clear, it's simpler to reason about when a callback that is passed between threads is destroyed. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode30 docs/callback.md:30: `RepeatingCallback<>` is created by `BindRepeating()` as a loose variant. Delete "as a loose variant". https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode31 docs/callback.md:31: Its internal storage is ref-counted and `RepeatingCallback<>` itself is a Technically, the internal storage of OnceCallback is also refcounted, it just never goes above 1, right? Perhaps reword this paragraph to: This is a callback variant that is copyable that can be run multiple times. It uses internal ref-counting to make copies cheap. However, since ownership is shared, it is harder to reason about when the callback and the bound state are destroyed, especially when the callback is passed between threads. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode36 docs/callback.md:36: The historycal `Callback<>` is `RepeatingCallback<>`. It's an alias of Typo: historical Perhaps word this as: The legacy `Callback<>` is currently aliased to `RepeatingCallback<>`. In new code, prefer to explicitly use `OnceCallback<>` where possible or `RepeatingCallback<>` otherwise. Once the migration is complete, the type alias will be removed and `OnceCallback<>` will be renamed to `Callback<>` to emphasize that it should be preferred. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode45 docs/callback.md:45: When you take a `Callback` object as a function argument, take it by value if Simplify this to: Pass `Callback` objects by value if ownership is transferred; otherwise, pass it by const-reference. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode49 docs/callback.md:49: // |Foo| just refers |cb|, but doesn't store it nor consume it. So the parameter |Foo| refers to |cb| but doesn't store it nor consume it. I think you can leave out the "So the parameter type..." or "Pass the Callback..." in this code comment and below, since it's clear from the code snippet. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode68 docs/callback.md:68: // |Qux| takes the ownership of |cb| and forwards it to PostTask, which also "transfers ownership to PostTask()." Otherwise, it sounds a bit like Qux and PostTask are both taking ownership of the same callback at the same time. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:130: the callback object, and to ensure that, it can be invoked only from rvalue consumes the callback object and can only be invoked on a callback rvalue. I think the last sentence (Use std::move on OnceCallback::Run...) can be removed, since it's clear from the code sample. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:206: A parameter of `Bind()` is moved into its internal storage if you use What if the parameter is already an rvalue, e.g. Bind(&Foo, CreateVector()) https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:216: the parameter of `BindRepeating()`, or if you use `BindOnce()`. This section is a bit confusing. Passed() can also be used with BindOnce() for the same effect, right? https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:260: Also, smart pointers (e.g. `std::unique_ptr<>`) are supported as the receiver. Nit: Smart pointers (e.g. ...) are also supported as the receiver. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:370: T&& Unwrap(const T&& obj) { Just curious: why is this a const rvalue reference? We won't be able to move out of |obj| since it's const, right? https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:377: checks if the receiver is null and cancels the invocation if it's null. You can Strictly speaking, we don't check if it's null: we do a boolean test to see if the pointer is still valid. Not sure how to word that here though. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:382: an argument wrapper such as Unretained, ConstRef, Owned, RetainedRef and Passed. Nit: wrap Unretained and the rest of the list items in `` https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:416: The `Bind` functions do the above using type-inference, and variadic templates. Nit: this is pre-existing, but remove the comma before "and" here.
https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode24 docs/callback.md:24: `OnceCallback<>` is created by `BindOnce()` as a restricted variant of On 2016/09/08 00:34:40, dcheng wrote: > `OnceCallback<>` is created by `BindOnce()`. This is a callback variant that is > a move-only type and can be run only once. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode26 docs/callback.md:26: movable types better as its bound parameter, and has clearer lifetime. Thus, On 2016/09/08 00:34:40, dcheng wrote: > Maybe clarify what 'better' means here. Perhaps omit the "and has clearer > lifetime" from this sentence as well: we can mention this in the following > sentence. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode27 docs/callback.md:27: thread hopping and result handning of an asynchronous operation are a good fit On 2016/09/08 00:34:40, dcheng wrote: > Typo: handling. But I would change this to indicate that we should default to > OnceCallback unless there's good reason not to: > > This should be the preferred callback type: since the lifetime of the callback > is clear, it's simpler to reason about when a callback that is passed between > threads is destroyed. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode30 docs/callback.md:30: `RepeatingCallback<>` is created by `BindRepeating()` as a loose variant. On 2016/09/08 00:34:39, dcheng wrote: > Delete "as a loose variant". Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode31 docs/callback.md:31: Its internal storage is ref-counted and `RepeatingCallback<>` itself is a On 2016/09/08 00:34:40, dcheng wrote: > Technically, the internal storage of OnceCallback is also refcounted, it just > never goes above 1, right? Right, removing the refcount gains the binary size on RepeatingCallback, and reduces it on OnceCallback. So, I'll remove it after most of Callbacks have migrated to OnceCallback. > Perhaps reword this paragraph to: > > This is a callback variant that is copyable that can be run multiple times. It > uses internal ref-counting to make copies cheap. However, since ownership is > shared, it is harder to reason about when the callback and the bound state are > destroyed, especially when the callback is passed between threads. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode36 docs/callback.md:36: The historycal `Callback<>` is `RepeatingCallback<>`. It's an alias of On 2016/09/08 00:34:40, dcheng wrote: > Typo: historical > > Perhaps word this as: > > The legacy `Callback<>` is currently aliased to `RepeatingCallback<>`. In new > code, prefer to explicitly use `OnceCallback<>` where possible or > `RepeatingCallback<>` otherwise. Once the migration is complete, the type alias > will be removed and `OnceCallback<>` will be renamed to `Callback<>` to > emphasize that it should be preferred. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode45 docs/callback.md:45: When you take a `Callback` object as a function argument, take it by value if On 2016/09/08 00:34:40, dcheng wrote: > Simplify this to: > > Pass `Callback` objects by value if ownership is transferred; otherwise, pass it > by const-reference. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode49 docs/callback.md:49: // |Foo| just refers |cb|, but doesn't store it nor consume it. So the parameter On 2016/09/08 00:34:40, dcheng wrote: > |Foo| refers to |cb| but doesn't store it nor consume it. > > I think you can leave out the "So the parameter type..." or "Pass the > Callback..." in this code comment and below, since it's clear from the code > snippet. OK, let me omit latter one. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcode68 docs/callback.md:68: // |Qux| takes the ownership of |cb| and forwards it to PostTask, which also On 2016/09/08 00:34:40, dcheng wrote: > "transfers ownership to PostTask()." > > Otherwise, it sounds a bit like Qux and PostTask are both taking ownership of > the same callback at the same time. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:130: the callback object, and to ensure that, it can be invoked only from rvalue On 2016/09/08 00:34:40, dcheng wrote: > consumes the callback object and can only be invoked on a callback rvalue. I > think the last sentence (Use std::move on OnceCallback::Run...) can be removed, > since it's clear from the code sample. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:206: A parameter of `Bind()` is moved into its internal storage if you use On 2016/09/08 00:34:39, dcheng wrote: > What if the parameter is already an rvalue, e.g. > > Bind(&Foo, CreateVector()) Hmm, let me rephrase it to just "if the parameter is passed as rvalue." and add an example for it. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:216: the parameter of `BindRepeating()`, or if you use `BindOnce()`. On 2016/09/08 00:34:40, dcheng wrote: > This section is a bit confusing. Passed() can also be used with BindOnce() for > the same effect, right? Right. Rephrased this to > If you use `BindOnce()`, the bound object is moved out even > without `Passed()`. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:260: Also, smart pointers (e.g. `std::unique_ptr<>`) are supported as the receiver. On 2016/09/08 00:34:40, dcheng wrote: > Nit: Smart pointers (e.g. ...) are also supported as the receiver. Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:370: T&& Unwrap(const T&& obj) { On 2016/09/08 00:34:40, dcheng wrote: > Just curious: why is this a const rvalue reference? We won't be able to move out > of |obj| since it's const, right? Oh, right it's a bug. Fixed. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:377: checks if the receiver is null and cancels the invocation if it's null. You can On 2016/09/08 00:34:40, dcheng wrote: > Strictly speaking, we don't check if it's null: we do a boolean test to see if > the pointer is still valid. Not sure how to word that here though. Replaced it with "checks if the receiver is evaluated to true". https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:382: an argument wrapper such as Unretained, ConstRef, Owned, RetainedRef and Passed. On 2016/09/08 00:34:40, dcheng wrote: > Nit: wrap Unretained and the rest of the list items in `` Done. https://codereview.chromium.org/2318113002/diff/20001/docs/callback.md#newcod... docs/callback.md:416: The `Bind` functions do the above using type-inference, and variadic templates. On 2016/09/08 00:34:40, dcheng wrote: > Nit: this is pre-existing, but remove the comma before "and" here. Done.
LGTM, thanks for improving the documentation! It covers a lot of the details that I always had trouble remembering. https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md#newcod... docs/callback.md:203: ### Move In and Move Out Random thought: maybe title this section "Avoiding Copies with Callback Parameters" or "Moving instead of Copying Callback Parameters" or "Binding Types without Copying", etc. I don't particularly like any of my suggestions, but the idea is to make it obvious that this section is for how to bind types without copying.
LGTM, just one nit https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md#newcode38 docs/callback.md:38: code, prefer to explicitly use `OnceCallback<>` where possible or "to explicitly use" => remove "or `RepeatingCallback<>` otherwise" => ", and use `RepeatingCallback<>` otherwise"
https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md#newcode38 docs/callback.md:38: code, prefer to explicitly use `OnceCallback<>` where possible or On 2016/10/05 06:15:53, Yuta Kitamura wrote: > "to explicitly use" => remove > > "or `RepeatingCallback<>` otherwise" > => ", and use `RepeatingCallback<>` otherwise" Done. https://codereview.chromium.org/2318113002/diff/40001/docs/callback.md#newcod... docs/callback.md:203: ### Move In and Move Out On 2016/10/04 22:26:25, dcheng wrote: > Random thought: maybe title this section "Avoiding Copies with Callback > Parameters" or "Moving instead of Copying Callback Parameters" or "Binding Types > without Copying", etc. > > I don't particularly like any of my suggestions, but the idea is to make it > obvious that this section is for how to bind types without copying. Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2318113002/#ps60001 (title: "minor rephrase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update //docs/callback.md for OnceCallback and RepeatingCallback ========== to ========== Update //docs/callback.md for OnceCallback and RepeatingCallback Committed: https://crrev.com/7c0c0cf17a2c77837916ecfe2f9d83712f69a7bc Cr-Commit-Position: refs/heads/master@{#423110} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7c0c0cf17a2c77837916ecfe2f9d83712f69a7bc Cr-Commit-Position: refs/heads/master@{#423110} |
