|
|
DescriptionAdd constexpr TaskTraits constructor.
The new constructor accepts any number of TaskTraits in any order.
E.g.
constexpr TaskTraits default_traits;
constexpr TaskTraits user_visible_traits = {TaskPriority::USER_VISIBLE};
constexpr TaskTraits user_visible_may_block_traits = {
TaskPriority::USER_VISIBLE, MayBlock()};
constexpr TaskTraits other_user_visible_may_block_traits = {
MayBlock(), TaskPriority::USER_VISIBLE};
BUG=713683
Review-Url: https://codereview.chromium.org/2829083002
Cr-Commit-Position: refs/heads/master@{#468417}
Committed: https://chromium.googlesource.com/chromium/src/+/879b2fcdbf4efc613abf3ab094ca11d53e74498c
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : fix android build error #Patch Set 5 : no-compile #Patch Set 6 : do-not-return-void-from-constexpr #Patch Set 7 : fix unused warning #
Total comments: 10
Patch Set 8 : CR-robliao-28-add-details-file #
Total comments: 24
Patch Set 9 : rebase #Patch Set 10 : CR-etipdoray-gab-36-38 #
Total comments: 4
Patch Set 11 : CR-robliao-48 #
Total comments: 2
Patch Set 12 : tag-dispatching-first #Patch Set 13 : tag-dispatching-first #Patch Set 14 : fix build error #Patch Set 15 : by-value #Patch Set 16 : delete-move-constructor #Patch Set 17 : fix-android-build-error #
Total comments: 1
Patch Set 18 : unretained #
Total comments: 4
Patch Set 19 : sfinae #
Total comments: 8
Patch Set 20 : CR gab #
Total comments: 6
Patch Set 21 : CR-gab #
Total comments: 2
Patch Set 22 : remove-comma #
Messages
Total messages: 119 (89 generated)
The CQ bit was checked by fdoray@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...
The CQ bit was checked by fdoray@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...
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fdoray@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...
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 fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
Pretty cool! A quick light pass while we get the file structure worked out (and then a deeper reading after that to preserve comment connectivity). https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:87: // WithBaseSyncPrimitives(). WithBaseSyncPrimitives() -> the WithBaseSyncPrimitives trait. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:107: // Avoid creating threads. Instead, use This guidance should probably be placed somewhere else. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:121: namespace internal { Place this is task_traits_details.h or task_traits_internals.h and including that here. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:124: // of type UnwantedType. The 2nd overload checks the type of the first argument Nit: 2nd -> second and 1st -> first
https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:121: namespace internal { On 2017/04/21 18:02:06, robliao wrote: > Place this is task_traits_details.h or task_traits_internals.h and including > that here. Wow, that didn't come out right at all. Place this internal namespace in task_traits_details.h or task_traits_internals.h and include that file here.
The CQ bit was checked by fdoray@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...
PTAnL https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:87: // WithBaseSyncPrimitives(). On 2017/04/21 18:02:06, robliao wrote: > WithBaseSyncPrimitives() -> the WithBaseSyncPrimitives trait. Done. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:107: // Avoid creating threads. Instead, use On 2017/04/21 18:02:06, robliao wrote: > This guidance should probably be placed somewhere else. Done. Writing a CL that adds it to base::Thread/base::PlatformThread right now. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:121: namespace internal { On 2017/04/21 18:03:13, robliao wrote: > On 2017/04/21 18:02:06, robliao wrote: > > Place this is task_traits_details.h or task_traits_internals.h and including > > that here. > > Wow, that didn't come out right at all. > Place this internal namespace in task_traits_details.h or > task_traits_internals.h and include that file here. Done. https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:124: // of type UnwantedType. The 2nd overload checks the type of the first argument On 2017/04/21 18:02:06, robliao wrote: > Nit: 2nd -> second and 1st -> first Done.
https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/110001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:107: // Avoid creating threads. Instead, use On 2017/04/21 20:35:20, fdoray wrote: > On 2017/04/21 18:02:06, robliao wrote: > > This guidance should probably be placed somewhere else. > > Done. Writing a CL that adds it to base::Thread/base::PlatformThread right now. https://codereview.chromium.org/2834783004/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
etipdoray@gmail.com changed reviewers: + etipdoray@gmail.com
I'm adding my opinion on CheckNoArgOfType. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:175: bool may_block() const { return may_block_; } Accessors could be constexpr as well. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:26: constexpr bool CheckNoArgOfType(const FirstArgType&, const ArgTypes&... args) { CheckNoArgOfType can be implemented without recursion, expending the parameter pack directly, which reduces compilation time as well as making it more readable. Something like: static_assert(meta_and<!std::is_same<UnwantedType, ArgTypes>::value...>::value, "..."); as for implementing meta_and (or something in the lines of c++17's std::conjunction), you can use a trick used by Louis Dionne in boost::hana: // true if meta_and<b1, b2, ...> is same as meta_and<true, true, ...> template <bool... b> struct meta_and : std::is_same<meta_and<b...>, meta_and<(b, true)...>> {}; https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:27: static_assert(!std::is_same<UnwantedType, FirstArgType>::value, It would be best to check instead if the associated Getter is callable with FirstArgType as argument, to support getters with possibly multiple overloads. However, is_callable (or is_invokable) is not part of c++11, so you would have to implement it. This might not be worth it, in which case you can ignore my comment.
gab@chromium.org changed reviewers: + ajwong@chromium.org
This is awesome, thanks :)! +ajwong https://codereview.chromium.org/2829083002/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:131: // calls back GetTraitFromArgList() with CallFirstTag() as first argument. Can you give a concrete example of the resolution order (order of constexpr "call" evaluations) for say {TaskPriority::USER_VISIBLE, MayBlock()}; ? https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:136: // MayBlock(), TaskPriority::USER_VISIBLE}; Add base:: namespace in examples for ease of copy/paste. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:162: // Deprecated. // Deprecated. Prefer constexpr construction to builder paradigm as documented above. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:181: TaskPriority priority_; // TODO(fdoray): Make these const after refactoring away deprecated builder pattern. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:12: Remove this empty line (consistent with EOF) https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:48: constexpr auto GetValueFromArgList(CallSecondTag, GetterType getter) I think constexpr GetterType GetValueFromArgList(CallSecondTag, GetterType getter) {} should compile (i.e. without auto -> decltype() syntax), since GetterType is fully defined in templated method FWIU the auto -> decltype() is useful when the type is derived from multiple types, e.g. template <typename T1, typename T2> auto compose(T1 a, T2 b) -> decltype(a + b); http://stackoverflow.com/questions/22514855/arrow-operator-in-function-heading https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:59: -> decltype(GetValueFromArgList(CallFirstTag(), getter, args...)) { Actually, isn't the return value of these 3 methods always typename GetterType::ValueType? Why does it need to be computed? https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:63: // Overload 3: Get value from first argument. Check that no argument in |args| Since the intent is for this one to match first and the other two to match as fallbacks I'd expect this one to be Overload 1 in this header. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:69: std::declval<GetterType>()(std::declval<FirstArgType>()))> Document in meta-comment of this file what GetterType is expected to provide. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits_unittest.nc (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_unittest.nc:27: #endif Also test unknown types (e.g. bool "false" or int "0") being passed in.
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fdoray@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...
PTAnL https://codereview.chromium.org/2829083002/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:131: // calls back GetTraitFromArgList() with CallFirstTag() as first argument. On 2017/04/25 17:48:14, gab wrote: > Can you give a concrete example of the resolution order (order of constexpr > "call" evaluations) for say > > {TaskPriority::USER_VISIBLE, MayBlock()}; > > ? Improved documentation. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:136: // MayBlock(), TaskPriority::USER_VISIBLE}; On 2017/04/25 17:48:14, gab wrote: > Add base:: namespace in examples for ease of copy/paste. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:162: // Deprecated. On 2017/04/25 17:48:14, gab wrote: > // Deprecated. Prefer constexpr construction to builder paradigm as documented > above. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:175: bool may_block() const { return may_block_; } On 2017/04/24 20:22:02, etipdoray wrote: > Accessors could be constexpr as well. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:181: TaskPriority priority_; On 2017/04/25 17:48:15, gab wrote: > // TODO(fdoray): Make these const after refactoring away deprecated builder > pattern. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:12: On 2017/04/25 17:48:15, gab wrote: > Remove this empty line (consistent with EOF) Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:26: constexpr bool CheckNoArgOfType(const FirstArgType&, const ArgTypes&... args) { On 2017/04/24 20:22:02, etipdoray wrote: > CheckNoArgOfType can be implemented without recursion, expending the parameter > pack directly, which reduces compilation time as well as making it more > readable. Something like: > > static_assert(meta_and<!std::is_same<UnwantedType, ArgTypes>::value...>::value, > "..."); > > as for implementing meta_and (or something in the lines of c++17's > std::conjunction), you can use a trick used by Louis Dionne in boost::hana: > > // true if meta_and<b1, b2, ...> is same as meta_and<true, true, ...> > template <bool... b> struct meta_and : > std::is_same<meta_and<b...>, meta_and<(b, true)...>> {}; Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:27: static_assert(!std::is_same<UnwantedType, FirstArgType>::value, On 2017/04/24 20:22:02, etipdoray wrote: > It would be best to check instead if the associated Getter is callable with > FirstArgType as argument, to support getters with possibly multiple overloads. > However, is_callable (or is_invokable) is not part of c++11, so you would have > to implement it. This might not be worth it, in which case you can ignore my > comment. not possible in c++11 https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:48: constexpr auto GetValueFromArgList(CallSecondTag, GetterType getter) On 2017/04/25 17:48:15, gab wrote: > I think > > constexpr GetterType GetValueFromArgList(CallSecondTag, GetterType getter) {} > > should compile (i.e. without auto -> decltype() syntax), since GetterType is > fully defined in templated method > > FWIU the auto -> decltype() is useful when the type is derived from multiple > types, e.g. > > template <typename T1, typename T2> > auto compose(T1 a, T2 b) -> decltype(a + b); > > http://stackoverflow.com/questions/22514855/arrow-operator-in-function-heading Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:59: -> decltype(GetValueFromArgList(CallFirstTag(), getter, args...)) { On 2017/04/25 17:48:15, gab wrote: > Actually, isn't the return value of these 3 methods always typename > GetterType::ValueType? Why does it need to be computed? Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:63: // Overload 3: Get value from first argument. Check that no argument in |args| On 2017/04/25 17:48:15, gab wrote: > Since the intent is for this one to match first and the other two to match as > fallbacks I'd expect this one to be Overload 1 in this header. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:69: std::declval<GetterType>()(std::declval<FirstArgType>()))> On 2017/04/25 17:48:15, gab wrote: > Document in meta-comment of this file what GetterType is expected to provide. Done. https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... File base/task_scheduler/task_traits_unittest.nc (right): https://codereview.chromium.org/2829083002/diff/130001/base/task_scheduler/ta... base/task_scheduler/task_traits_unittest.nc:27: #endif On 2017/04/25 17:48:15, gab wrote: > Also test unknown types (e.g. bool "false" or int "0") being passed in. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:196: // The constructor only accepts types that are convertible to ValidTrait. // ValidTrait ensures that we only accept types appropriate for TaskTraits. https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:65: struct CallFirstTag : CallSecondTag {}; To make sure I understand this correctly, these are only used to force calling into Overload 1, right? Since all external callers will always call (GetValueFromArgList(CallFirstTag(), ...), it would be nicer to do a GetValueFromArgList(GetterType getter, const FirstArgType& first_arg, const ARgTypes&... args) And then have that call GetValueFromArgListImpl(CallFirstTag() This way the notion of the tag doesn't need to be exposed outside this file.
The CQ bit was checked by fdoray@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...
please take another look https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:196: // The constructor only accepts types that are convertible to ValidTrait. On 2017/04/26 19:45:54, robliao wrote: > // ValidTrait ensures that we only accept types appropriate for TaskTraits. Done. https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:65: struct CallFirstTag : CallSecondTag {}; On 2017/04/26 19:45:54, robliao wrote: > To make sure I understand this correctly, these are only used to force calling > into Overload 1, right? > > Since all external callers will always call (GetValueFromArgList(CallFirstTag(), > ...), it would be nicer to do a > > GetValueFromArgList(GetterType getter, const FirstArgType& first_arg, const > ARgTypes&... args) > > And then have that call > > GetValueFromArgListImpl(CallFirstTag() > > This way the notion of the tag doesn't need to be exposed outside this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm % fixes for compiler. https://codereview.chromium.org/2829083002/diff/190001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/190001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:74: struct CallSecondTag {}; These will unfortunately need to be moved upwards so that GetValueFromArgList can see it.
The CQ bit was checked by fdoray@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/2829083002/diff/190001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/190001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:74: struct CallSecondTag {}; On 2017/04/26 21:15:54, robliao wrote: > These will unfortunately need to be moved upwards so that GetValueFromArgList > can see it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_asan_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 fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
There seems to be some problems with overloading constructors. Here's a possible solution! https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:137: constexpr TaskTraits(ArgTypes... args) To avoid getting this constructor in the way during overload resolution (which seems to confuse gcc), you could make use of SFINAE to cause an early discard (the static_assert would then be useless). You can do this by adding an extra (dummy) template parameter: typename std::enable_if< internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value >::type* = nullptr or, to make it more readable (we have to assign a default value to it, with the default type being void): internal::if_t< internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > = true with template <bool Condition> using if_t = typename std::enable_if<Condition, bool>::type
On 2017/04/27 22:27:39, etipdoray wrote: > There seems to be some problems with overloading constructors. Here's a possible > solution! > > https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... > File base/task_scheduler/task_traits.h (right): > > https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... > base/task_scheduler/task_traits.h:137: constexpr TaskTraits(ArgTypes... args) > To avoid getting this constructor in the way during overload resolution (which > seems to confuse gcc), you could make use of SFINAE to cause an early discard > (the static_assert would then be useless). You can do this by adding an extra > (dummy) template parameter: > > typename std::enable_if< > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > >::type* = nullptr > > or, to make it more readable (we have to assign a default value to it, with the > default type being void): > > internal::if_t< > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > = true > > with > > template <bool Condition> > using if_t = typename std::enable_if<Condition, bool>::type I think we're getting close to needing to deal with https://google.github.io/styleguide/cppguide.html#Template_metaprogramming if we're talking about SFINAE. What's the current issue?
On 2017/04/27 22:29:37, robliao wrote: > On 2017/04/27 22:27:39, etipdoray wrote: > > There seems to be some problems with overloading constructors. Here's a > possible > > solution! > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... > > File base/task_scheduler/task_traits.h (right): > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/task_scheduler/ta... > > base/task_scheduler/task_traits.h:137: constexpr TaskTraits(ArgTypes... args) > > To avoid getting this constructor in the way during overload resolution (which > > seems to confuse gcc), you could make use of SFINAE to cause an early discard > > (the static_assert would then be useless). You can do this by adding an extra > > (dummy) template parameter: > > > > typename std::enable_if< > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > >::type* = nullptr > > > > or, to make it more readable (we have to assign a default value to it, with > the > > default type being void): > > > > internal::if_t< > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > > = true > > > > with > > > > template <bool Condition> > > using if_t = typename std::enable_if<Condition, bool>::type > > I think we're getting close to needing to deal with > https://google.github.io/styleguide/cppguide.html#Template_metaprogramming > if we're talking about SFINAE. > > What's the current issue? In some cases, the traits constructor is preferred over the copy constructor (with ConstRefWrapper<base::TaskTraits> as argument), when we would expect the opposite (or maybe i'm wrong?). Alternatively, the ambiguity could be solved by 1- Always giving an exact match, that is, avoiding calling the copy constructor with a wrapper as argument 2- Modifying the traits constructor to take a tag as first argument, similar to std::piecewise_construct_t, although this would add noise to the API
On Thu, Apr 27, 2017 at 6:49 PM, <etipdoray@gmail.com> wrote: > On 2017/04/27 22:29:37, robliao wrote: > > On 2017/04/27 22:27:39, etipdoray wrote: > > > There seems to be some problems with overloading constructors. Here's a > > possible > > > solution! > > > > > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/ > task_scheduler/task_traits.h > > > File base/task_scheduler/task_traits.h (right): > > > > > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/ > task_scheduler/task_traits.h#newcode137 > > > base/task_scheduler/task_traits.h:137: constexpr > TaskTraits(ArgTypes... > args) > > > To avoid getting this constructor in the way during overload resolution > (which > > > seems to confuse gcc), you could make use of SFINAE to cause an early > discard > > > (the static_assert would then be useless). You can do this by adding an > extra > > > (dummy) template parameter: > > > > > > typename std::enable_if< > > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > > >::type* = nullptr > > > > > > or, to make it more readable (we have to assign a default value to it, > with > > the > > > default type being void): > > > > > > internal::if_t< > > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > > > = true > > > > > > with > > > > > > template <bool Condition> > > > using if_t = typename std::enable_if<Condition, bool>::type > > > > I think we're getting close to needing to deal with > > https://google.github.io/styleguide/cppguide.html#Template_ > metaprogramming > > if we're talking about SFINAE. > > > > What's the current issue? > > In some cases, the traits constructor is preferred over the copy > constructor > (with ConstRefWrapper<base::TaskTraits> as argument), when we would > expect the > opposite (or maybe i'm wrong?). > ConstRefWrapper is used when you specify base::ConstRef() on a bound argument in a Callback, saying that you want to store it inside the Callback as a reference (actually a pointer in the implementation) instead of copying it into the Callback's storage. If that method receives it by value it would copy out of the Callback though. Is that what you're seeing? > Alternatively, the ambiguity could be solved by > 1- Always giving an exact match, that is, avoiding calling the copy > constructor > with a wrapper as argument > 2- Modifying the traits constructor to take a tag as first argument, > similar to > std::piecewise_construct_t, although this would add noise to the API > > > https://codereview.chromium.org/2829083002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/27 22:56:35, danakj wrote: > On Thu, Apr 27, 2017 at 6:49 PM, <mailto:etipdoray@gmail.com> wrote: > > > On 2017/04/27 22:29:37, robliao wrote: > > > On 2017/04/27 22:27:39, etipdoray wrote: > > > > There seems to be some problems with overloading constructors. Here's a > > > possible > > > > solution! > > > > > > > > > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/ > > task_scheduler/task_traits.h > > > > File base/task_scheduler/task_traits.h (right): > > > > > > > > > > > > > https://codereview.chromium.org/2829083002/diff/310001/base/ > > task_scheduler/task_traits.h#newcode137 > > > > base/task_scheduler/task_traits.h:137: constexpr > > TaskTraits(ArgTypes... > > args) > > > > To avoid getting this constructor in the way during overload resolution > > (which > > > > seems to confuse gcc), you could make use of SFINAE to cause an early > > discard > > > > (the static_assert would then be useless). You can do this by adding an > > extra > > > > (dummy) template parameter: > > > > > > > > typename std::enable_if< > > > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > > > >::type* = nullptr > > > > > > > > or, to make it more readable (we have to assign a default value to it, > > with > > > the > > > > default type being void): > > > > > > > > internal::if_t< > > > > internal::ArgsAreConvertible<ValidTrait, ArgTypes...>::value > > > > > = true > > > > > > > > with > > > > > > > > template <bool Condition> > > > > using if_t = typename std::enable_if<Condition, bool>::type > > > > > > I think we're getting close to needing to deal with > > > https://google.github.io/styleguide/cppguide.html#Template_ > > metaprogramming > > > if we're talking about SFINAE. > > > > > > What's the current issue? > > > > In some cases, the traits constructor is preferred over the copy > > constructor > > (with ConstRefWrapper<base::TaskTraits> as argument), when we would > > expect the > > opposite (or maybe i'm wrong?). > > > > ConstRefWrapper is used when you specify base::ConstRef() on a bound > argument in a Callback, saying that you want to store it inside the > Callback as a reference (actually a pointer in the implementation) instead > of copying it into the Callback's storage. If that method receives it by > value it would copy out of the Callback though. Is that what you're seeing? > > > > Alternatively, the ambiguity could be solved by > > 1- Always giving an exact match, that is, avoiding calling the copy > > constructor > > with a wrapper as argument > > 2- Modifying the traits constructor to take a tag as first argument, > > similar to > > std::piecewise_construct_t, although this would add noise to the API > > > > > > https://codereview.chromium.org/2829083002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I was referring to linux_android_rel_ng failure in task_scheduler_impl_unittest. I don't think ConstRefWrapper had much to do with it after all. It seems like the tuple implementation from ndk didn't mix well with constructors from TaskTraits, though Francois found a workaround!
lgtm
After investigation, it seems like the problem is that on iOS and Android, copying an std::tuple<TaskTraits> involves instantiating TaskTraits::TaskTraits<std::tuple<TaskTraits>>. This instantiation causes a static_assert failure (see the error generated when building https://codereview.chromium.org/2847113002/ on iOS below). To avoid this, we would need to make sure that TaskTraits::TaskTraits<std::tuple<TaskTraits>> is not defined (currently, it is defined, but it generates a static_assert failure). That could be achieved with sfinae. Since copying an std::tuple<TaskTraits> is not common, I'll keep this improvement for a separate CL. Note that using TaskTraits in base::Bind/BindOnce involves copying a std::tuple<TaskTraits>. ../../base/task_scheduler/task_traits.h:155:5: error: static_assert failed "Argument of invalid type provided to the constructor of TaskTraits." static_assert( ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/type_traits:880:66: note: in instantiation of function template specialization 'base::TaskTraits::TaskTraits<std::__1::tuple<base::TaskTraits> >' requested here : public integral_constant<bool, __is_convertible_to(_T1, _T2) && ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tuple:266:32: note: in instantiation of template class 'std::__1::is_convertible<std::__1::tuple<base::TaskTraits> &, base::TaskTraits>' requested here is_convertible<_Tp0, _Up0>::value && ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tuple:278:12: note: in instantiation of template class 'std::__1::__tuple_convertible_imp<std::__1::__tuple_types<std::__1::tuple<base::TaskTraits> &>, std::__1::__tuple_types<base::TaskTraits> >' requested here : public __tuple_convertible_imp< ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tuple:291:14: note: in instantiation of template class 'std::__1::__tuple_convertible_apply<true, std::__1::tuple<std::__1::tuple<base::TaskTraits> &>, std::__1::__tuple_types<base::TaskTraits> >' requested here : public __tuple_convertible_apply<tuple_size<typename remove_reference<_Tp>::type>::value == ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/tuple:543:26: note: in instantiation of template class 'std::__1::__tuple_convertible<std::__1::tuple<std::__1::tuple<base::TaskTraits> &>, std::__1::__tuple_types<base::TaskTraits>, true, true>' requested here __tuple_convertible ^ /b/build/slave/ios-device-xcode-clang/build/src/build/ios_files/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/tuple:561:9: note: while substituting prior template arguments into non-type template parameter [with _Up = <std::__1::tuple<base::TaskTraits> &>] tuple(_Up&&... __u) ^~~~~ ../../base/task_scheduler/task_traits_unittest.cc:15:26: note: while substituting deduced template arguments into function template 'tuple' [with _Up = <std::__1::tuple<base::TaskTraits> &>, $1 = (no value)] std::tuple<TaskTraits> other_tuple = tuple; ^
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2829083002/#ps320001 (title: "unretained")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
nits on impl, looking at latest version now after discussion to make Bind() work. https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:192: // ValidTrait ensures that we only accept types appropriate for TaskTraits. s/we only accept types appropriate for TaskTraits/TaskTraits' constructor only accepts appropriate types/ https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:81: CallSecondTag, I guess it doesn't matter that this is CallFirstTag or CallSecondTag, right? It's the only signature that matches an empty list of |args|. So I'd say make this the first overload with a CallFirstTag, then the more advanced one the second overload with CallFirstTag, only the one dropping the param needs a CallSecondTag? That makes it easier to understand the tags IMO. i.e.: 1) Default when |args| is empty. 2) Get arg from first arg. 3) Skip first arg and go back to (1) if (2) failed.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Nice, I like that approach even better to check param types (since it allows Bind()). For those following along: we discussed and Francois found a way to replace the static_assert that types are valid with a template check which makes the constructor not exist if an initializer list param is invalid And the error message is actually pretty straight forward (effectively points the reader to TaskTraits' constructor documentation): c:\src\chromium\src\base\task_scheduler\task_traits_unittest.cc(12): error C2440: 'initializing': cannot convert from 'initializer list' to 'base::TaskTraits' c:\src\chromium\src\base\task_scheduler\task_traits_unittest.cc(12): note: No constructor could take the source type, or constructor overload resolution was ambiguous https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:223: Unretained(&GetParam().traits), Unretained(&task_ran)), Bring back old Bind? https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:29: struct ArgsAreConvertible<CheckedType, FirstArgType, ArgTypes...> Unused https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:128: // Allows instantiation of multiple types in one statement. Expand comment to explain what this is used for. https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_traits_unittest.nc (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_unittest.nc:27: #elif defined(NCTEST_TASK_TRAITS_INVALID_TYPE) // [r"Argument of invalid type provided to the constructor of TaskTraits."] Update regex
The CQ bit was checked by fdoray@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...
ptanl https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:192: // ValidTrait ensures that we only accept types appropriate for TaskTraits. On 2017/04/28 18:03:40, gab wrote: > s/we only accept types appropriate for TaskTraits/TaskTraits' constructor only > accepts appropriate types/ Done. https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/320001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:81: CallSecondTag, On 2017/04/28 18:03:40, gab wrote: > I guess it doesn't matter that this is CallFirstTag or CallSecondTag, right? > It's the only signature that matches an empty list of |args|. > > So I'd say make this the first overload with a CallFirstTag, then the more > advanced one the second overload with CallFirstTag, only the one dropping the > param needs a CallSecondTag? That makes it easier to understand the tags IMO. > > i.e.: > 1) Default when |args| is empty. > 2) Get arg from first arg. > 3) Skip first arg and go back to (1) if (2) failed. Done. https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:223: Unretained(&GetParam().traits), Unretained(&task_ran)), On 2017/04/28 18:15:42, gab wrote: > Bring back old Bind? Done. https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:29: struct ArgsAreConvertible<CheckedType, FirstArgType, ArgTypes...> On 2017/04/28 18:15:43, gab wrote: > Unused Done. https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:128: // Allows instantiation of multiple types in one statement. On 2017/04/28 18:15:42, gab wrote: > Expand comment to explain what this is used for. Done. https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... File base/task_scheduler/task_traits_unittest.nc (right): https://codereview.chromium.org/2829083002/diff/340001/base/task_scheduler/ta... base/task_scheduler/task_traits_unittest.nc:27: #elif defined(NCTEST_TASK_TRAITS_INVALID_TYPE) // [r"Argument of invalid type provided to the constructor of TaskTraits."] On 2017/04/28 18:15:43, gab wrote: > Update regex Done.
https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:122: ValidTrait(base::WithBaseSyncPrimitives) {} Don't need base::? https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:135: // TaskPriority, TaskShutdownBehavior, MayBlock and/or WithBaseSyncPrimitives , before "and/or" https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:71: CallFirstTag, I meant to reorder this to be Overload 1 so that logic and comment flows as : 1) Is list empty -> return default 2) Is first arg of right type -> return value 3) Discard first arg and go back to (1)
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 fdoray@chromium.org to run a CQ dry run
gab@: PTAnL https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:122: ValidTrait(base::WithBaseSyncPrimitives) {} On 2017/04/28 19:00:31, gab wrote: > Don't need base::? Yes, to avoid conflict with TaskTraits::MayBlock(), TaskTraits::WithBaseSyncPrimitives(). https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:135: // TaskPriority, TaskShutdownBehavior, MayBlock and/or WithBaseSyncPrimitives On 2017/04/28 19:00:31, gab wrote: > , before "and/or" Done. https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:71: CallFirstTag, On 2017/04/28 19:00:31, gab wrote: > I meant to reorder this to be Overload 1 so that logic and comment flows as : > > 1) Is list empty -> return default > 2) Is first arg of right type -> return value > 3) Discard first arg and go back to (1) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ nit, this is awesome :)! https://codereview.chromium.org/2829083002/diff/380001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/380001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:28: // prefers using the second overload, because the type of the first argument no "," before "because" IMO
https://codereview.chromium.org/2829083002/diff/380001/base/task_scheduler/ta... File base/task_scheduler/task_traits_details.h (right): https://codereview.chromium.org/2829083002/diff/380001/base/task_scheduler/ta... base/task_scheduler/task_traits_details.h:28: // prefers using the second overload, because the type of the first argument On 2017/05/01 18:56:17, gab wrote: > no "," before "because" IMO Done.
The CQ bit was checked by fdoray@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...
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, etipdoray@gmail.com, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2829083002/#ps400001 (title: "remove-comma")
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": 400001, "attempt_start_ts": 1493667038743840, "parent_rev": "89ac4eb510183e617a186af25a4a94afe11ace92", "commit_rev": "879b2fcdbf4efc613abf3ab094ca11d53e74498c"}
Message was sent while issue was closed.
Description was changed from ========== Add constexpr TaskTraits constructor. The new constructor accepts any number of TaskTraits in any order. E.g. constexpr TaskTraits default_traits; constexpr TaskTraits user_visible_traits = {TaskPriority::USER_VISIBLE}; constexpr TaskTraits user_visible_may_block_traits = { TaskPriority::USER_VISIBLE, MayBlock()}; constexpr TaskTraits other_user_visible_may_block_traits = { MayBlock(), TaskPriority::USER_VISIBLE}; BUG=713683 ========== to ========== Add constexpr TaskTraits constructor. The new constructor accepts any number of TaskTraits in any order. E.g. constexpr TaskTraits default_traits; constexpr TaskTraits user_visible_traits = {TaskPriority::USER_VISIBLE}; constexpr TaskTraits user_visible_may_block_traits = { TaskPriority::USER_VISIBLE, MayBlock()}; constexpr TaskTraits other_user_visible_may_block_traits = { MayBlock(), TaskPriority::USER_VISIBLE}; BUG=713683 Review-Url: https://codereview.chromium.org/2829083002 Cr-Commit-Position: refs/heads/master@{#468417} Committed: https://chromium.googlesource.com/chromium/src/+/879b2fcdbf4efc613abf3ab094ca... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:400001) as https://chromium.googlesource.com/chromium/src/+/879b2fcdbf4efc613abf3ab094ca... |