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

[Albert Astals Cid]

> 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.
> 
> Ingo Klöcker wrote:
>     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.

You admit you even haven't look at the code so it is clear we are arguing because we both like it.
I did not introduce any problem, i fixed one, you say that if somebody makes the method public or calls close again then it will crash, yes, but it will be that person that introduces the problem, not me. You can not expect to touch code (and much less something called "close") without reviewing that it does.


- Albert


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