Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517793002:
Improve compile error when invoking OnceCallback::Run on a non-rvalue. (Closed)
DescriptionImprove compile error when invoking OnceCallback::Run on a non-rvalue.
Some callback helper templates have also been simplified.
BUG=667031
Committed: https://crrev.com/77172b96f4e84e91ace073cfacfcd9a50a3d96d8
Cr-Commit-Position: refs/heads/master@{#433810}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add C++ language lawyering explanation. #
Total comments: 1
Messages
Total messages: 26 (19 generated)
Description was changed from ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. BUG=667031 ========== to ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. Some callback helper templates have also been simplified. BUG=667031 ==========
dcheng@chromium.org changed reviewers: + danakj@chromium.org, tzik@chromium.org
The CQ bit was checked by dcheng@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...
https://codereview.chromium.org/2517793002/diff/1/base/bind_unittest.nc File base/bind_unittest.nc (right): https://codereview.chromium.org/2517793002/diff/1/base/bind_unittest.nc#newco... base/bind_unittest.nc:231: cb.Run(); I could put this in callback_unittest.nc as well, but it feels weird to put in a test case that's invalid for multiple reasons (since it's invalid to call Run() on an unbound callback). https://codereview.chromium.org/2517793002/diff/1/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2517793002/diff/1/base/callback.h#newcode44 base/callback.h:44: R Run(Args... args) & { All overloads must either use ref-qualifiers or not use ref-qualifiers, so this must be ref-qualified with &. https://codereview.chromium.org/2517793002/diff/1/base/callback.h#newcode45 base/callback.h:45: static_assert(!IsOnceCallback<CallbackType>::value, This feels a bit silly, but we need to make sure the static_assert is dependent on the template parameters.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/2517793002/diff/1/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2517793002/diff/1/base/callback.h#newcode45 base/callback.h:45: static_assert(!IsOnceCallback<CallbackType>::value, On 2016/11/19 08:07:59, dcheng wrote: > This feels a bit silly, but we need to make sure the static_assert is dependent > on the template parameters. 14.7.1 of the standard is so dense that I don't understand why this needs to be the case :) Can you leave a comment with what you said here, so that future us don't try to optimize this into a "false"
The CQ bit was checked by dcheng@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...
https://codereview.chromium.org/2517793002/diff/1/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2517793002/diff/1/base/callback.h#newcode45 base/callback.h:45: static_assert(!IsOnceCallback<CallbackType>::value, On 2016/11/21 20:00:40, vmpstr wrote: > On 2016/11/19 08:07:59, dcheng wrote: > > This feels a bit silly, but we need to make sure the static_assert is > dependent > > on the template parameters. > > 14.7.1 of the standard is so dense that I don't understand why this needs to be > the case :) Can you leave a comment with what you said here, so that future us > don't try to optimize this into a "false" Added a language lawyering explanation; hopefully it kind of makes sense.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dcheng@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...
lgtm https://codereview.chromium.org/2517793002/diff/20001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2517793002/diff/20001/base/callback.h#newcode57 base/callback.h:57: "std::move(callback).Run()."); Hmm, OK, let's go this way. I prefer just deleting it rather than disabling it by static_assert. However, the error message for this looks valuable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479800536850390,
"parent_rev": "e7c0ea7174fa8a86f962258f0adb4142f3e55cc7", "commit_rev":
"d822c18d7c606d01c39fe3de1340ad67c41448cb"}
Message was sent while issue was closed.
Description was changed from ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. Some callback helper templates have also been simplified. BUG=667031 ========== to ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. Some callback helper templates have also been simplified. BUG=667031 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. Some callback helper templates have also been simplified. BUG=667031 ========== to ========== Improve compile error when invoking OnceCallback::Run on a non-rvalue. Some callback helper templates have also been simplified. BUG=667031 Committed: https://crrev.com/77172b96f4e84e91ace073cfacfcd9a50a3d96d8 Cr-Commit-Position: refs/heads/master@{#433810} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/77172b96f4e84e91ace073cfacfcd9a50a3d96d8 Cr-Commit-Position: refs/heads/master@{#433810} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
