[Kde-pim] Re: Review Request: Do not leak m_prev_time

[Ingo Klöcker]

> On July 17, 2011, 9:32 p.m., Ingo Klöcker wrote:
> > It would have been much saner to make m_prev_time a utimbuf (instead of a utimbuf *). Using a pointer is just calling for problems (like forgotten deletes or double deletes). Your patch opens the door for a double delete because you do not set m_prev_time to 0 after deleting it. Please fix, preferably by making m_prev_time a utimbuf. Alternatively, make it a QScopedPointer<utimbuf>. QScopedPointer takes care of resetting to 0 (if you call reset()) and it takes care of deletion (if ReadMBox is destroyed).
> 
> Albert Astals Cid wrote:
>     i do not need to set it to 0 since that function is only called on destruction of the object.
> 
> Ingo Klöcker wrote:
>     You delete it inside the method close(). Even if close() is currently a private method (I haven't checked whether that's the case) somebody could make it a public method with the next commit and call it explicitly. Maybe even twice. BOOM! Double deletion. There are good reasons for resetting pointers to 0 after deletion. There is no reason not to do so.
> 
> Albert Astals Cid wrote:
>     Sincerely, i do not know why you keep arguing with me instead of fixing it.

I'm wondering why you post a patch for review, but refuse to accept suggestions for improvement. Okay, you have already pushed your patch. Nevertheless, IMHO it's your responsibility to fix the problem I have pointed out since you have introduced this problem.


- Ingo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101929/#review4793
-----------------------------------------------------------


On July 12, 2011, 12:44 p.m., Albert Astals Cid wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101929/
> -----------------------------------------------------------
> 
> (Updated July 12, 2011, 12:44 p.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Summary
> -------
> 
> Seems we are not deleting m_prev_time
> 
> 
> Diffs
> -----
> 
>   kioslave/mbox/readmbox.cpp d244f2c 
> 
> Diff: http://git.reviewboard.kde.org/r/101929/diff
> 
> 
> Testing
> -------
> 
> None
> 
> 
> Thanks,
> 
> Albert
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/