Hacker News new | ask | show | jobs
by mhogomchungu 3143 days ago
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.

3 comments

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.
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

[2] https://github.com/mhogomchungu/tasks/blob/a1512a1b5e0392a06...

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.
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/4210a8fad57958fad...

You can change the `std::result_of_t<...>` for `typename std::result_of<...>::type` to make it work in C++11 :)
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.

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.

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.