Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1283183003:
Use forwarding with SkTLazy::init. (Closed)
|
Created:
5 years, 4 months ago by bungeman-skia Modified:
5 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionUse forwarding with SkTLazy::init.
This allows removal the difficult to use (and so currently unused)
placement new and related macros to allow any constructor of T to
be used to initilize the storage of SkTLazy. This also properly
aligns the SkTLazy storage.
Committed: https://skia.googlesource.com/skia/+/72440a3785c13b8ec539d7e11bea1124eeddecbd
Patch Set 1 #
Total comments: 2
Patch Set 2 : Also remove no longer needed constructor. #Patch Set 3 : Add && in deducing context. #
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283183003/1
bungeman@google.com changed reviewers: + bsalomon@google.com, mtklein@google.com
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 bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283183003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283183003/20001
On 2015/08/12 19:40:53, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1283183003/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1283183003/20001 lgtm
https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h File include/core/SkTLazy.h (right): https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:49: template <typename... Args> T* init(Args... args) { make_shared looks like this: template <typename T, typename... Args> std::shared_ptr<T> make_shared(Args&&... args) { ... std::forward<Args>(args)... ... } Do we not similarly need `Args&&... args` as the parameter list here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/12 19:45:59, mtklein wrote: > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h > File include/core/SkTLazy.h (right): > > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h#newc... > include/core/SkTLazy.h:49: template <typename... Args> T* init(Args... args) { > make_shared looks like this: > > template <typename T, typename... Args> > std::shared_ptr<T> make_shared(Args&&... args) { > ... > std::forward<Args>(args)... > ... > } > > Do we not similarly need `Args&&... args` as the parameter list here? Good question... if so, we probably need to update SkFunction.h (from which I admit this was cribbed because I forgot the exact pack syntax).
On 2015/08/12 20:00:29, bungeman1 wrote: > On 2015/08/12 19:45:59, mtklein wrote: > > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h > > File include/core/SkTLazy.h (right): > > > > > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h#newc... > > include/core/SkTLazy.h:49: template <typename... Args> T* init(Args... args) { > > make_shared looks like this: > > > > template <typename T, typename... Args> > > std::shared_ptr<T> make_shared(Args&&... args) { > > ... > > std::forward<Args>(args)... > > ... > > } > > > > Do we not similarly need `Args&&... args` as the parameter list here? > > Good question... if so, we probably need to update SkFunction.h (from which I > admit this was cribbed because I forgot the exact pack syntax). Yeah, color me confused: std::function::operator() R operator()( ArgTypes... args ) const; template< class T, class... Args > shared_ptr<T> make_shared( Args&&... args );
On 2015/08/12 20:00:29, bungeman1 wrote: > On 2015/08/12 19:45:59, mtklein wrote: > > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h > > File include/core/SkTLazy.h (right): > > > > > https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h#newc... > > include/core/SkTLazy.h:49: template <typename... Args> T* init(Args... args) { > > make_shared looks like this: > > > > template <typename T, typename... Args> > > std::shared_ptr<T> make_shared(Args&&... args) { > > ... > > std::forward<Args>(args)... > > ... > > } > > > > Do we not similarly need `Args&&... args` as the parameter list here? > > Good question... if so, we probably need to update SkFunction.h (from which I > admit this was cribbed because I forgot the exact pack syntax). Ah, yes, type deducing context...
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283183003/40001
https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h File include/core/SkTLazy.h (right): https://codereview.chromium.org/1283183003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:49: template <typename... Args> T* init(Args... args) { On 2015/08/12 19:45:59, mtklein wrote: > make_shared looks like this: > > template <typename T, typename... Args> > std::shared_ptr<T> make_shared(Args&&... args) { > ... > std::forward<Args>(args)... > ... > } > > Do we not similarly need `Args&&... args` as the parameter list here? Yes, we do, since this is a deducing context. I think SkFunction is all ok, because Args is never in a deducing context. (The Args are always required on the struct, and are not deduced.)
lgtm
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 bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1283183003/#ps40001 (title: "Add && in deducing context.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283183003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/72440a3785c13b8ec539d7e11bea1124eeddecbd |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
