Jonathon Jongsma wrote: > Andre Moreira Magalhaes wrote: >> First version of the proposal with all suggestions is up on my shared >> branch: >> http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/shared >> >> >> >> BR >> Andre > > OK, I took a look at the current implementation, and here are my > notes. It's obviously possible that i've misunderstood or > misinterpreted something, so feel free to challenge me on any of these. > > [explicit inline SharedPtr(T *d) : d(d) { if (d) { d->ref(); } }] > boost::shared_ptr uses a templated constructor for this case (e.g. > template > <class Y> SharedPtr(Y *d) ...) which allows it to somehow remember the > actual > pointer type passed so that if it was a different type that is > convertible to > T*, it will call delete with its original type and thus will not leak > even if Y > does not have a virtual destructor). Granted, they probably use some > black magic > to accomplish this, but it might be worth considering. For example, > consider > this contrived example: > class A {}; > class B : public A { > public: > B() : x(new int[1000]) {} > ~B() { delete [] x; } > private: > int * x; > } > // This will leak 1000*sizeof(int) when it goes out of scope > SharedPtr<A> shared(new B()); > // whereas this does not leak: > boost::shared_ptr<A> boost_shared(new B()); > > [SharedData] > Speaking of virtual destructors, SharedData should probably have one > if it's meant to > be the base class of all ref-counted classes. Also, the name is rather > > [template<class X> > inline SharedPtr(const SharedPtr<X> &o)] > I don't think this constructor is a good idea. I assume this is > designed so that > you can easily convert from a derived class pointer to a base class > pointer as you > would with raw pointers, but I think it's much better to make the > programmer state > explicitly what they want to do. And in any case, dynamic_cast is > probably the more > appropriate cast in this situation. See also > boost::static_pointer_cast/dynamic_pointer_cast/etc or > Glib::RefPtr::cast_static/cast_dynamic/etc > > [inline SharedPtr(const WeakPtr<T> &o)] > It's probably best to make this explicit as well so that converting > from a weak > pointer to a shared pointer is only done when it's intentional. You > also might > consider something like the boost::weak_ptr::lock() API that allows > you to > easily create a SharedPtr from a WeakPtr > > [Copy constructor and operator=()] > You might consider using the swap() idiom like Glib::RefPtr uses, it's > quite > elegant and allows greater code reuse: > http://svn.gnome.org/viewvc/glibmm/trunk/glib/glibmm/refptr.h?view=markup#l228 > > > > [~SharedPtr()] > Perhaps ~SharedPtr() should just call reset()? You'd be doing an > extra 'd = 0' > operation during destruction, but you'd eliminate some code duplication. > > [data()] > You may want to think about whether data() is really necessary as a > public API. > It can certainly be useful at times to access the underlying raw > pointer, but it > can also encourage improper use by application developers, since they > can easily > grab the pointer out and store it somewhere, which circumvents the > reference-counting > > [General] > Be very careful about passing smart pointers as const references since > that > circumvents the smart pointer's reference-counting mechanism. It > looks like > everywhere you use const references in the implementation of the smart > pointer > are ok, but just keep it in mind (especially when you actually use > these classes > other Telepathy API) > > You should make your API reference crystal clear that this class is > only for use > with classes derived from SharedData. No matter how obvious you think > it is, > people will still assume that it's a generic smart pointer that they > can use > with their arbitrary classes (this is from my experience with glibmm) > > I don't know if the goal is for this class to be threadsafe or not, > but strictly > speaking, it's not. The reference count involves indirection, so > although the > the integer operations are atomic, the following code is not atomic as > a whole: > 'if (d) {d->ref();}'. So while the reference count itself can not be > corrupted, there are corner cases that could cause problems: (e.g. > thread A > could test that d is not null, then another thread could decrement the > ref count > to zero and delete d, and then the thread A would attempt to call > d->ref()). > > Thanks for the review, I will implement the suggestions and send for review again.
BR Andrunko _______________________________________________ telepathy mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/telepathy
