D21703: [Transaction] Replace template for functor with std::function

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:5b6fd8fe88b7: [Transaction] Replace template for functor 
with std::function (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21703?vs=59467=59512

REVISION DETAIL
  https://phabricator.kde.org/D21703

AFFECTED FILES
  src/engine/transaction.h
  src/engine/writetransaction.cpp
  src/engine/writetransaction.h

To: bruns, #baloo, ngraham, astippich, poboiko
Cc: kde-frameworks-devel, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21703: [Transaction] Replace template for functor with std::function

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Yep, seems sensible.

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

REVISION DETAIL
  https://phabricator.kde.org/D21703

To: bruns, #baloo, ngraham, astippich, poboiko
Cc: kde-frameworks-devel, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21703: [Transaction] Replace template for functor with std::function

[Stefan Brüns]

bruns added a comment.


  In D21703#477158 , @poboiko wrote:
  
  > I thought about it myself. I googled it a bit (i.e. here 
) and saw 
that there might be some quite unwanted runtime overhead because of using 
`std::function`. It might be negligible (since we're doing some costly DB 
operations inside anyways), but I'd prefer if we did some profiling to make 
sure it's OK.
  
  
  There are two causes mentioned why the template may be faster:
  
  1. the compiler may be able to inline or even remove a trivial template
  2. the std::function may need to heap-allocate the space for the bound values 
(either for captured values of a lambda, or when using std::bind)
  
  Neither applies here. (1.) is obviously not the case.
  
  (2.) does not apply, as the lambda is bound outside the loop, nothing has to 
be copied or allocated in the loop.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D21703

To: bruns, #baloo, ngraham, astippich, poboiko
Cc: kde-frameworks-devel, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21703: [Transaction] Replace template for functor with std::function

[Igor Poboiko]

poboiko added a comment.


  I thought about it myself. I googled it a bit (i.e. here 
) and saw 
that there might be some quite unwanted runtime overhead because of using 
`std::function`. It might be negligible (since we're doing some costly DB 
operations inside anyways), but I'd prefer if we did some profiling to make 
sure it's OK.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D21703

To: bruns, #baloo, ngraham, astippich, poboiko
Cc: kde-frameworks-devel, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21703: [Transaction] Replace template for functor with std::function

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The callback parameter has a specific type, it accepts a document ID and
  returns bool, while the template is overly broad.

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

REVISION DETAIL
  https://phabricator.kde.org/D21703

AFFECTED FILES
  src/engine/transaction.h
  src/engine/writetransaction.cpp
  src/engine/writetransaction.h

To: bruns, #baloo, ngraham, astippich, poboiko
Cc: kde-frameworks-devel, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams