Review Request 111043: Fix possibly memory leak in sidewinder code by not called destructor on incomplete type of QTextDocument

[Friedrich W. H. Kossebau]

> On June 23, 2013, 2:30 a.m., Inge Wallin wrote:
> > filters/sheets/excel/sidewinder/excel.h, line 811
> > <http://git.reviewboard.kde.org/r/111043/diff/1/?file=150601#file150601line811>
> >
> >     I like the private class method that you suggested, but in that case we shouldn't have any inline functions either.

Would agree. But here all the other classes in this header also have inline functions, so I chose consistency :)
(inline makes even less sense with virtual methods)

Might be fixed/changed in a separate patch. Unrelated to this.


> On June 23, 2013, 2:30 a.m., Inge Wallin wrote:
> > filters/sheets/excel/sidewinder/excel.cpp, line 1683
> > <http://git.reviewboard.kde.org/r/111043/diff/1/?file=150602#file150602line1683>
> >
> >     Urk!  Magic numbers!

Indeed. But not topic of this patch :)


> On June 23, 2013, 2:30 a.m., Inge Wallin wrote:
> > filters/sheets/excel/sidewinder/excel.h, line 800
> > <http://git.reviewboard.kde.org/r/111043/diff/1/?file=150601#file150601line800>
> >
> >     This may just be a personal reference, but in the context of calligra I prefer if we don't use "doc", "document" or similar in any other context than an ODF document.
> >     
> >     Especially in Words and the text classes in the libraries it has given me much confusion when a "document" is mentioned and it actually means a QTextDocument which is a container for some rich text. Can we use names like "textContents" or (in this case) "richText" or just "text" instead?

Thanks for comments. Good idea, richText is way better here, changed to that.


- Friedrich W. H.


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


On June 27, 2013, 10:37 p.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111043/
> -----------------------------------------------------------
> 
> (Updated June 27, 2013, 10:37 p.m.)
> 
> 
> Review request for Calligra, Marijn Kruisselbrink and Sebastian Sauer.
> 
> 
> Description
> -------
> 
> TxORecord is deconstructed in places where QTextDocument is not a completely defined type (because that is only forward declared in the header with the declaration of TxORecord, excel.h. Delete on objects of incomplete type is just undefined, so not an error usually, thus the compiler will not stop on it, see e.g. http://en.wikibooks.org/wiki/More_C++_Idioms/Checked_delete for the background.
> Which means all QTextDocument members of TxORecord are currently leaked.
> 
> The most simple fix would have been to just remove the forward declaration of <QTextDocument>  with a #include <QTextDocument> in excel.h, so that QTextDocument is completely defined.
> 
> But somehow I was tempted to instead move the members to a private class, like all/most of the other record classes have, that way also moving the deletion of the QTextDocument member to records.cpp, where the type is completely defined.
> 
> Whatever you prefer, your choice, just tell.
> 
> Okay to backport to 2.7?
> 
> 
> Diffs
> -----
> 
>   filters/sheets/excel/import/ODrawClient.cpp c558d73 
>   filters/sheets/excel/sidewinder/excel.h 5b0076c 
>   filters/sheets/excel/sidewinder/excel.cpp 50f54c0 
>   filters/sheets/excel/sidewinder/worksheetsubstreamhandler.cpp bd66048 
> 
> Diff: http://git.reviewboard.kde.org/r/111043/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130627/afe3d2a3/attachment.htm>