[Scummvm-devel] thread safe container objects (Was: [Scummvm-cvs-logs] SF.net SVN: scummvm: [28914] scummvm/trunk/engines/lure/res_struct.h)

[Max Horn]

Am 16.09.2007 um 06:03 schrieb dreammaster at users.sourceforge.net:

> Revision: 28914
>           http://scummvm.svn.sourceforge.net/scummvm/? 
> rev=28914&view=rev
> Author:   dreammaster
> Date:     2007-09-15 21:03:50 -0700 (Sat, 15 Sep 2007)
>
> Log Message:
> -----------
> Changed ManagedList to make it more thread safe

Hi there,

hm, that commit should be taken with a grain of salt, I'd say.  
Basically, that commit message doesn't quite make sense, does it?  
Either you are thread safe, or you aren't, there's no such thing as  
"a bit thread safe". In this particular case, the new code still  
isn't thread safe. None of our stock container classes is thread  
safe, in fact (and that is not likely to change). If you need thread  
safe access to a container, then either make the code accessing it  
use a mutex; or subclass the container (which you already do here,  
luckily), and let it keep (and use!) its own mutex. That incurs some  
overhead, of course, but that's what you get when you want to access  
& modify a structure from multiple threads.

As it is, I think the change is problematic, because instead of  
fixing the problem, it just hides it / makes it less likely to be  
triggered. So, assuming the old code was causing buggy behavior, you  
just went from a hard-to-catch bug to a near-impossible-to-catch  
"heisenbug", which IMO makes things worse, not better :-(.


Cheers,
Max

>
> Modified Paths:
> --------------
>     scummvm/trunk/engines/lure/res_struct.h
>
> Modified: scummvm/trunk/engines/lure/res_struct.h
> ===================================================================
> --- scummvm/trunk/engines/lure/res_struct.h	2007-09-16 03:20:10 UTC  
> (rev 28913)
> +++ scummvm/trunk/engines/lure/res_struct.h	2007-09-16 04:03:50 UTC  
> (rev 28914)
> @@ -254,23 +254,28 @@
>  	}
>
>  	void clear() {
> -		typename Common::List<T>::iterator i;
> -		for (i = Common_List::begin(); i != Common_List::end(); ++i)
> -			delete *i;
> -		Common_List::clear();		
> +		typename Common_List::iterator i = Common_List::begin();
> +		while (i != Common_List::end()) {
> +			T v = *i;
> +			i = Common_List::erase(i);
> +			delete v;
> +		}
>  	}
>
> -	typename Common::List<T>::iterator erase(typename  
> Common::List<T>::iterator pos) {
> -		delete *pos;
> -		return Common_List::erase(pos);
> +	typename Common_List::iterator erase(typename  
> Common_List::iterator pos) {
> +		T obj = *pos;
> +		typename Common_List::iterator result = Common_List::erase(pos);
> +		delete obj;
> +		return result;
>  	}
>
> -	typename Common::List<T>::iterator erase(typename  
> Common::List<T>::iterator first,
> -			typename Common::List<T>::iterator last) {
> -		typename Common::List<T>::iterator i;
> -		for (i = first; i != last; ++i)
> -			delete *i;
> -		return Common_List::erase(first, last);
> +	typename Common_List::iterator erase(typename  
> Common_List::iterator first,
> +			typename Common_List::iterator last) {
> +
> +		while (first != last)
> +			erase(first++);
> +
> +		return last;
>  	}
>  };
>
>
>
> This was sent by the SourceForge.net collaborative development  
> platform, the world's largest Open Source development site.
>
> ---------------------------------------------------------------------- 
> ---
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> Scummvm-cvs-logs mailing list
> Scummvm-cvs-logs at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-cvs-logs
>