4 ms·

There could be an efficiency bug here. 1. "item->id()" is designed to return a pointer. 2. Consumer of the API writes their code to take a copy of the pointer

There could be an efficiency bug here. 1. "item->id()" is designed to return a pointer. 2. Consumer of the API writes their code to take a copy of the pointer and passes it along to "processSomeItem". It is cheap to make a copy of a pointer 3. The API is changed and it now returns a reference. 4. The assignment to "itemId" now makes an expensive copy of the object behind the reference causing inefficiency bug. 5. "processSomeItem()" API can handle the copy and the code compiles and runs without the problem being noticed.

ChrisSD 9y ago
I'm not sure that it's safe to use an unstable API without being very careful and understanding the breaking changes before updating it.
arximboldi 9y ago
Actually performance problems are way more probable when not using auto. Example: std::function<void()> = [] { ... }; Vs: auto = [] { ... } In the first case you are doing type erasure, which adds quite few penalties. Even in other cases, the type you typed might be convertible from the actual thing that is returned, causing extra conversions. If you always use "auto" the chances you use the right type and do less conversions is way higher.
- mhogomchungu 9y ago
  sometimes,declaring lambdas with auto is not not sufficient as i discovered in my "tasks"[1] project. An example place where auto breaks is here[2]. [1] https://github.com/mhogomchungu/tasks https://github.com/mhogomchungu/tasks [2] https://github.com/mhogomchungu/tasks/blob/a1512a1b5e0392a069305a779e1e2834ca67d94d/task.h#L896-L916 https://github.com/mhogomchungu/tasks/blob/a1512a1b5e0392a06...
  - arximboldi 9y ago
    The problem there is in your Task::run() function. It now looks like this in the commit I just saw: template< typename T,typename ... Args > future<T>& run( std::function< T( Args ... ) > function,Args ... args ) { return Task::run<T>( std::bind( std::move( function ),std::move( args ) ... ) ) ; } It should look like this for that example to work without the type erasure (haven't tried, there might be typos): template< typename Fn,typename ... Args > future<std::result_of_t(Fn(Args...))>& run( Fn&&, Args&& ... args ) { return Task::run<std::result_of_t(Fn(Args...))>( std::bind( std::forward<Fn>( function ),std::forward<Args>( args ) ... ) ) ; } This version is also more efficient, since otherwise you are type erasing twice.
    - mhogomchungu 9y ago
      Your code failed to compile but this one[1] succeeds. Side effect of your way is that it now makes the project requiring C++14 but i can live with that. [1] https://github.com/mhogomchungu/tasks/blob/4210a8fad57958fad58c60a1a98e35208217cee6/task.h#L598 https://github.com/mhogomchungu/tasks/blob/4210a8fad57958fad...
      - arximboldi 9y ago
        You can change the `std::result_of_t<...>` for `typename std::result_of<...>::type` to make it work in C++11 :)
        mhogomchungu 9y ago
        All my projects are C++14 so it wasnt an issue for me but i liked being c++11 for other people and i am glad there is a way to get it back to c++11. Already committed your suggestion. Thank you very much.
AnimalMuppet 9y ago
I'd expect in that situation that step 3 produces a compiler error at the call to processSomeItem(). (Yeah, they could have overloaded processSomeItem...) Also, passing stuff by reference does not make a copy.

ChrisSD 9y ago

I'm not sure that it's safe to use an unstable API without being very careful and understanding the breaking changes before updating it.

arximboldi 9y ago

Actually performance problems are way more probable when not using auto. Example: std::function<void()> = [] { ... }; Vs: auto = [] { ... } In the first case you are doing type erasure, which adds quite few penalties. Even in other cases, the type you typed might be convertible from the actual thing that is returned, causing extra conversions. If you always use "auto" the chances you use the right type and do less conversions is way higher.

mhogomchungu 9y ago
sometimes,declaring lambdas with auto is not not sufficient as i discovered in my "tasks"[1] project. An example place where auto breaks is here[2]. [1] https://github.com/mhogomchungu/tasks https://github.com/mhogomchungu/tasks [2] https://github.com/mhogomchungu/tasks/blob/a1512a1b5e0392a069305a779e1e2834ca67d94d/task.h#L896-L916 https://github.com/mhogomchungu/tasks/blob/a1512a1b5e0392a06...
- arximboldi 9y ago
  The problem there is in your Task::run() function. It now looks like this in the commit I just saw: template< typename T,typename ... Args > future<T>& run( std::function< T( Args ... ) > function,Args ... args ) { return Task::run<T>( std::bind( std::move( function ),std::move( args ) ... ) ) ; } It should look like this for that example to work without the type erasure (haven't tried, there might be typos): template< typename Fn,typename ... Args > future<std::result_of_t(Fn(Args...))>& run( Fn&&, Args&& ... args ) { return Task::run<std::result_of_t(Fn(Args...))>( std::bind( std::forward<Fn>( function ),std::forward<Args>( args ) ... ) ) ; } This version is also more efficient, since otherwise you are type erasing twice.
  - mhogomchungu 9y ago
    Your code failed to compile but this one[1] succeeds. Side effect of your way is that it now makes the project requiring C++14 but i can live with that. [1] https://github.com/mhogomchungu/tasks/blob/4210a8fad57958fad58c60a1a98e35208217cee6/task.h#L598 https://github.com/mhogomchungu/tasks/blob/4210a8fad57958fad...
    - arximboldi 9y ago
      You can change the `std::result_of_t<...>` for `typename std::result_of<...>::type` to make it work in C++11 :)
      - mhogomchungu 9y ago
        All my projects are C++14 so it wasnt an issue for me but i liked being c++11 for other people and i am glad there is a way to get it back to c++11. Already committed your suggestion. Thank you very much.

AnimalMuppet 9y ago

I'd expect in that situation that step 3 produces a compiler error at the call to processSomeItem(). (Yeah, they could have overloaded processSomeItem...) Also, passing stuff by reference does not make a copy.