Re: bk commit into 5.1 tree (hezx:1.2534) BUG#33029

[He Zhenxing]

Hi Andrei

Thank you for your review, it helped me to improve the patch a lot.

On 2008-03-13 Thu 11:49 +0200，Andrei Elkin wrote：
> Zhen Xing, hello.
> 
> The patches are okay to me.
> 
> Thanks for answering my question on #rep today.
> 
> I see my remark previous remark about the default copy constructor was
> not entirely correct.
> I am glad the idea was accepted and the new methods plus the copy
> constuctor should be a good contribution.
> 
> Great that you found yourself a slipped from my attention
> rpl_master_has_bug()!
> 
> Thanks for doing it!
> 
> cheers,
> 
> Andrei
> 
> <...  removed  ...>
> 
> >> 
> >> Why not to be content with the existing default copy constructor
> >> instead of your new methods?
> >> 
> >> E.g
> >> 
> >>  
> auto_inc_intervals_forced.save(&backup->auto_inc_intervals_forced);
> >> 
> >> turns to be 2 instuctions
> >> 
> >>   backup->auto_inc_intervals_forced= auto_inc_intervals_forced;
> >>   auto_inc_intervals_forced.empty_no_free();
> >> 
> >> but i'd rather to go with them as empty_no_free() is a public.
> >> 
> >
> > There is no copy constructor, only a defalut constructor that takes no
> > argument, correct me if I am wrong.
> >
> > I had thought of use the code you mentioned, the problem is I think a
> > simple copy construct that just copy all the members is not safe, I mean
> > after:
> >
> >    backup->auto_inc_intervals_forced= auto_inc_intervals_forced;
> >
> > We will have two instance with shared list of elements, if we use empty
> > on any of the instance, the other will be in a dangling state, all the
> > elements are actuall freed. In the current case, this is not a problem,
> > because we can call empty_no_free to empty the original instance without
> > free. But when we add a new interface, it can be use later by other
> > code. So this would be an unsafe interface.
> >
> > If we do need the copy construct, we have to add a reference count to
> > record how many copies we have, and only free the list when the count is
> > zero. Maybe we should try this.
> >
> > BTW: I think empty_no_free should be private instead of public.
> >
>