SAL_NO_VTABLE in formula
Hey, so by going through Lsan reports I noted that we have a few classes in formula that are marked with SAL_NO_VTABLE and therefore have no virtual protected destructors, This prevents us from deleting some of these instances and it looks like people just leaked them in the past. Is there any reason not to remove the SAL_NO_VTABLE and make the destructor virtual and public. I"m talking especially about include/formula/IFunctionDescription.hxx where the use of SAL_NO_VTABLE looks like premature optimization to me. Regards, Markus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: SAL_NO_VTABLE in formula
Hi Markus, On Friday, 2014-05-23 03:14:30 +0200, Markus Mohrhard wrote: > so by going through Lsan reports I noted that we have a few classes in > formula that are marked with SAL_NO_VTABLE and therefore have no virtual > protected destructors, This prevents us from deleting some of these > instances and it looks like people just leaked them in the past. What actually leaks, given that these classes have no member variables and only define interfaces as pure abstract base classes one derives from? > Is there any reason not to remove the SAL_NO_VTABLE and make the destructor > virtual and public. I"m talking especially about > include/formula/IFunctionDescription.hxx where the use of SAL_NO_VTABLE > looks like premature optimization to me. This appears to me as exactly what the comment on SAL_NO_VTABLE in include/sal/types.h talks about. But no, if we really leak because of SAL_NO_VTABLE (this is on Windows, isn't it? because it's defined empty for other platforms) then I don't object to remove it, but then we should also remove the SAL_NO_VTABLE define. However, is it a prerequisite to have a non-virtual dtor when using SAL_NO_VTABLE? Or wouldn't adding a virtual to the dtor already solve the problem and not make Lsan stumble about? Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key ID: 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Support the FSFE, care about Free Software! https://fsfe.org/support/?erack pgpKN9UBzlugG.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: SAL_NO_VTABLE in formula
Hey, On Fri, May 23, 2014 at 1:24 PM, Eike Rathke wrote: > Hi Markus, > > On Friday, 2014-05-23 03:14:30 +0200, Markus Mohrhard wrote: > > > so by going through Lsan reports I noted that we have a few classes in > > formula that are marked with SAL_NO_VTABLE and therefore have no virtual > > protected destructors, This prevents us from deleting some of these > > instances and it looks like people just leaked them in the past. > > What actually leaks, given that these classes have no member variables > and only define interfaces as pure abstract base classes one derives > from? > There is code in formula which generates objects from sc but can of course only use the abstract interfaces. Instead of deleting the objects we just leak them after use because the d'tor is protected. > > > Is there any reason not to remove the SAL_NO_VTABLE and make the > destructor > > virtual and public. I"m talking especially about > > include/formula/IFunctionDescription.hxx where the use of SAL_NO_VTABLE > > looks like premature optimization to me. > > This appears to me as exactly what the comment on SAL_NO_VTABLE in > include/sal/types.h talks about. > The main question is if it really makes a difference. I understand that it makes a difference for objects where we create thousands or more but these classes seem to generate just a few objects. > > But no, if we really leak because of SAL_NO_VTABLE (this is on Windows, > isn't it? because it's defined empty for other platforms) then I don't > object to remove it, but then we should also remove the SAL_NO_VTABLE > define. > > However, is it a prerequisite to have a non-virtual dtor when using > SAL_NO_VTABLE? Or wouldn't adding a virtual to the dtor already solve > the problem and not make Lsan stumble about? > How can you use a virtual destructor when you don't have a v-table? As far as my understanding goes the destructors are protected and non-virtual because you can't have a virtual destructor and should not be able to delete the objects through the base class. So the question from my point of view is more if it there is really a good reason to save these few bytes per object? Personally I would not worry about the space a v-table allocates until I'm really desperate and don't have any other place to optimize. Regards, Markus > > Eike > > -- > LibreOffice Calc developer. Number formatter stricken i18n > transpositionizer. > GPG key ID: 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A > Support the FSFE, care about Free Software! > https://fsfe.org/support/?erack > ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: SAL_NO_VTABLE in formula
On 05/23/2014 01:47 PM, Markus Mohrhard wrote: On Fri, May 23, 2014 at 1:24 PM, Eike Rathke mailto:er...@redhat.com>> wrote: On Friday, 2014-05-23 03:14:30 +0200, Markus Mohrhard wrote: > so by going through Lsan reports I noted that we have a few classes in > formula that are marked with SAL_NO_VTABLE and therefore have no virtual > protected destructors, This prevents us from deleting some of these > instances and it looks like people just leaked them in the past. What actually leaks, given that these classes have no member variables and only define interfaces as pure abstract base classes one derives from? There is code in formula which generates objects from sc but can of course only use the abstract interfaces. Instead of deleting the objects we just leak them after use because the d'tor is protected. If client code wants to polymorphically delete through IFunctionDescription (which I understand it does), then IFunctionDescription of course needs a public virtual dtor. Whether or not to decorate IFunctionDescription with __declspec(novtable) (AKA SAL_NO_VTABLE) on Windows should be an orthogonal decision (at least, that's my---poor, as it is about Windows after all---understanding of __declspec(novtable)). Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
