Re: [dev] List of 2730 uncallable methods in DEV300_m10
Kohei Yoshida wrote: On Fri, 2008-05-09 at 20:01 +0200, Mathias Bauer wrote: Another interesting discovery was that removing ctors can be dangerous at times because some compilers automatically create default or copy ctors for classes even if they aren't used. One technique to work around this is to put the default and copy ctors in the private section of the class declaration, and leave their definitions out. That should prevent the compiler from automatically generating those ctors, and if any code constructs that class via default or copy constructor, then the link should fail. Yes, this is always a good practice. In the current situation the problem didn't happen in the class where the ctor was removed but in some derived classes. These classes formerly had a ctor (and so it wasn't necessary to declare a default ctor, be it private or not) that was removed in the patch and now some compiler thought it might be a good idea to create a default ctor (and didn't find one in the base class that still had another ctor but no default ctor). A good example for your approach to have a look before applying the patch. :-) And perhaps a hint that always adding a (private) default ctor and copy ctor is something to think about. I found removing the whole class better than playing with declared (but not implemented) default ctors. At the end a class whose ctor is never called can't be much useful. :-) Ciao, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to [EMAIL PROTECTED]. I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Hi Mathias, On Tue, 2008-05-20 at 08:43 +0200, Mathias Bauer wrote: In the current situation the problem didn't happen in the class where the ctor was removed but in some derived classes. These classes formerly had a ctor (and so it wasn't necessary to declare a default ctor, be it private or not) that was removed in the patch and now some compiler thought it might be a good idea to create a default ctor (and didn't find one in the base class that still had another ctor but no default ctor). Ok. Yeah, this can be very hard to backtrack when it causes a problem later on. A good example for your approach to have a look before applying the patch. :-) And perhaps a hint that always adding a (private) default ctor and copy ctor is something to think about. Good point. I found removing the whole class better than playing with declared (but not implemented) default ctors. At the end a class whose ctor is never called can't be much useful. :-) Yes. I've also removed a couple of classes instead of just removing the ctor(s) for the reason you already mentioned. So, I definitely agree with you there. As you said, the default ctor not being called is often an indication that the class as a whole is not used at all. Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] CHINA001 (was: Re: [dev] List of 2730 uncallable methods in DEV300_m10)
On Fri, 2008-05-09 at 17:54 -0400, Kohei Yoshida wrote: Hi Niklas, On Fri, 2008-05-09 at 19:17 +0200, Niklas Nebel wrote: example: CHINA001 In the interest of removing the unused code, I'd like know what those CHINA001 labels are for. Is it okay to perhaps review those commented out lines and see if we can remove them permanently? Its connected to the binfilter stuff, comments in the non-binfilter code with CHINA001 should basically indicate that the code was removed from the non-binfilter code because it was believed only needed to support other-code that is now solely in the binfilter, i.e. the legacy 5.2 binfilter filters and other similar things. C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Hi Kohei, On Thursday, 2008-05-08 00:22:10 -0400, Kohei Yoshida wrote: If I recall correctly, the module with the most unused methods that doesn't have anything in the pipeline to remove them is sc, so there's where the lowest hanging fruit should be. Since no one has raised hands, let me tackle that. Thanks a lot! Just a note though: please don't blindly remove everything that appears to be unused, e.g. while ScCompressedArray::GetPrevValue() is currently unused it is the counterpart of GetNextValue() and IMHO should be kept for completeness of implementation. It is also some inline template code, so shouldn't hurt either. Removing FillDataArray() on the other hand is fine because it was used in just one special scenario. Thanks Eike -- OOo/SO Calc core developer. Number formatter stricken i18n transpositionizer. SunSign 0x87F8D412 : 2F58 5236 DB02 F335 8304 7D6C 65C9 F9B5 87F8 D412 OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't send personal mail to the [EMAIL PROTECTED] account, which I use for mailing lists only and don't read from outside Sun. Use [EMAIL PROTECTED] Thanks. pgpBgs4zeBuRZ.pgp Description: PGP signature
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 16:47 +0200, Eike Rathke wrote: while ScCompressedArray::GetPrevValue() is currently unused it is the counterpart of GetNextValue() and IMHO should be kept for completeness of implementation. Perhaps #ifdef FUTURE around it, or else I can add such things to the whitelist if that's undesirable. C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 16:47 +0200, Eike Rathke wrote: Just a note though: please don't blindly remove everything that appears to be unused, e.g. while ScCompressedArray::GetPrevValue() is currently unused it is the counterpart of GetNextValue() and IMHO should be kept for completeness of implementation. It is also some inline template code, so shouldn't hurt either. Removing FillDataArray() on the other hand is fine because it was used in just one special scenario. Done! I've restored ScCompressedArray::GetPrevValue(). I'll be more careful about removing one of getter-setter pair. Let me know if there is more that I've removed but you want to restore. Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 16:11 +0100, Caolan McNamara wrote: On Fri, 2008-05-09 at 16:47 +0200, Eike Rathke wrote: while ScCompressedArray::GetPrevValue() is currently unused it is the counterpart of GetNextValue() and IMHO should be kept for completeness of implementation. Perhaps #ifdef FUTURE around it, or else I can add such things to the whitelist if that's undesirable. For now, I'll put '#define MAYBE_REMOVE_THIS 0' in global.hxx and use it for the codes I'm not sure about. Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Caolan McNamara wrote: On Fri, 2008-05-09 at 16:47 +0200, Eike Rathke wrote: while ScCompressedArray::GetPrevValue() is currently unused it is the counterpart of GetNextValue() and IMHO should be kept for completeness of implementation. Perhaps #ifdef FUTURE around it, or else I can add such things to the whitelist if that's undesirable. Good old opt-in vs. opt-out choice. I'd prefer to leave all methods in the header files, commented out and marked somehow, unless someone has (manually) determined that they really shouldn't be there. Otherwise, inevitably, it will cause problems much later, where you look at a class and ask yourself, how is that supposed to work, I can (for example) add an entry but not remove it. I know these marks are ugly and tend to stay there forever (example: CHINA001), but it's still better than always having to dig through the CVS log to see if the class you're looking at is still the class as it was intended to be. Niklas - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Hi Malte, Malte Timmermann wrote: Hi Caolan, thanks for the updated list! :) I wonder: Who dares / volunteers to simply remove these methods? This is ongoing already. I have a CWS where the biggest chunks have been put in already (binfilter, filter, svtools, filter, desktop...). CWS is mba30patches01 and is still in QA. A warning to everyone committing such cleanups to cvs: please use non-pro builds on at least one platform (--enable-dbgutil in configure) as I found some cases where the stripped code only compiled for pro builds. Without a non-pro build I had broke the master for all developers using non-pro versions. Another interesting discovery was that removing ctors can be dangerous at times because some compilers automatically create default or copy ctors for classes even if they aren't used. If the corresponding ctor of the base class was removed, the build broke. Easiest fix for that was removing the classes completely as a class that isn't constructed surely also isn't really used, even if the methods are stilled called somewhere else in obviously also superfluous code. I found this problem mainly in binfilter - a fact that doesn't surprise a lot. I didn't find a pattern for when which compiler chose which class, I had broken builds on all platforms for different classes! Ciao, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to [EMAIL PROTECTED]. I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 20:01 +0200, Mathias Bauer wrote: A warning to everyone committing such cleanups to cvs: please use non-pro builds on at least one platform Indeed. The unused methods are always pulled from a .pro build. The same issue arises with stuff used only on one platform but included in all platforms, e.g. some ole2 stuff under windows. Another interesting discovery was that removing ctors can be dangerous at times because some compilers automatically create default or copy ctors for classes even if they aren't used. ... I found this problem mainly in binfilter - a fact that doesn't surprise a lot. Yeah, given the rather poor state of standalone C++ parsing frameworks the current mechanism is a somewhat horrific (but functional) scraping of the output gcc assembly stage. The output data is correct, but there is the limitation that it's not easy to tell that a specific ctor of a range of ctors is unused vs that objects of that class are impossible to actually create at all even if other code accepts pointers to such objects and call methods on them. FWIW http://people.redhat.com/caolanm/GoOOoCon08.odp has some brief notes on it. C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 19:17 +0200, Niklas Nebel wrote: Good old opt-in vs. opt-out choice. I'd prefer to leave all methods in the header files, commented out and marked somehow, unless someone has (manually) determined that they really shouldn't be there. I can start using REMOVE_THIS macro and use it like this: #if REMOVE_THIS void RemoveMe(); #endif for all future removals. I've already removed quite a bit so I can't go back and do this for the code that's already been removed, though. I also don't remove code blindly; I at least spend some time to take a brief look at the method and check its references before removing it, to make sure it is in fact ok to remove it, or if it is desirable to leave it in. Having said that, I would like to still reserve the right to just outright remove code if I think with strong certainty that the code shouldn't be there at all. ;-) Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 15:16 -0400, Kohei Yoshida wrote: Having said that, I would like to still reserve the right to just outright remove code if I think with strong certainty that the code shouldn't be there at all. ;-) And again, if I removed something by mistake that you want to leave in, please let me know, and I'll restore it back. Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Fri, 2008-05-09 at 20:01 +0200, Mathias Bauer wrote: Another interesting discovery was that removing ctors can be dangerous at times because some compilers automatically create default or copy ctors for classes even if they aren't used. One technique to work around this is to put the default and copy ctors in the private section of the class declaration, and leave their definitions out. That should prevent the compiler from automatically generating those ctors, and if any code constructs that class via default or copy constructor, then the link should fail. -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[dev] CHINA001 (was: Re: [dev] List of 2730 uncallable methods in DEV300_m10)
Hi Niklas, On Fri, 2008-05-09 at 19:17 +0200, Niklas Nebel wrote: example: CHINA001 In the interest of removing the unused code, I'd like know what those CHINA001 labels are for. Is it okay to perhaps review those commented out lines and see if we can remove them permanently? Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Thu, 2008-05-08 at 00:22 -0400, Kohei Yoshida wrote: On Tue, 2008-05-06 at 09:10 +0100, Caolan McNamara wrote: If I recall correctly, the module with the most unused methods that doesn't have anything in the pipeline to remove them is sc, so there's where the lowest hanging fruit should be. Since no one has raised hands, let me tackle that. The patch at #i85185# removes a handful of these methods just to get it started. C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Tue, 2008-05-06 at 09:10 +0100, Caolan McNamara wrote: If I recall correctly, the module with the most unused methods that doesn't have anything in the pipeline to remove them is sc, so there's where the lowest hanging fruit should be. Since no one has raised hands, let me tackle that. Kohei -- Kohei Yoshida - OpenOffice.org Engineer - Novell, Inc. [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[dev] List of 2730 uncallable methods in DEV300_m10
See http://people.redhat.com/caolanm/callcatcher/DEV300_m10/ for full list. Top three offenders are... 1521 binfilter 403 sc 198 sd C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Hi Caolan, See http://people.redhat.com/caolanm/callcatcher/DEV300_m10/ for full list. Top three offenders are... sorry for the dumb question, but what are uncallable methods? Ciao Frank -- - Frank Schönheit, Software Engineer [EMAIL PROTECTED] - - Sun Microsystems http://www.sun.com/staroffice - - OpenOffice.org Base http://dba.openoffice.org - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
Methods with very strange names, hard to speak ;) No just kidding - it means that very likely (100%?) these methods are never used/called from somewhere. Malte. Frank Schönheit - Sun Microsystems Germany wrote, On 06.05.08 09:51: Hi Caolan, See http://people.redhat.com/caolanm/callcatcher/DEV300_m10/ for full list. Top three offenders are... sorry for the dumb question, but what are uncallable methods? Ciao Frank - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] List of 2730 uncallable methods in DEV300_m10
On Tue, 2008-05-06 at 09:54 +0200, Malte Timmermann wrote: Methods with very strange names, hard to speak ;) No just kidding - it means that very likely (100%?) these methods are never used/called from somewhere. *cough*, yes. Maybe a what the hell are they, might be useful. They're the non-virtual methods in those modules that are not referenced by anything at all, i.e. nothing calls those methods so they would appear to be dead code. Auto-excluded from the list are well-formed operators that should be encouraged, i.e. unused assignment operators and copy constructors are excluded as are const variants of methods if there is a used non-const variant. Also excluded are the unused methods in those modules that arise from using the various OOo macros that create loads of generally unused DeleteAndDestroy and _ForEach etc methods. Of course, there might be some reason to hold onto those symbols, i.e. a) they are symbols which are dlopened somewhere else in OOo, maybe the case for about 1% of them, i.e. only the handful of C symbols at the top of the binfilter list b) they are debugging-only symbols which should be inside some #if OSL_DEBUG X line c) they are used on some other platform, but just not linux, and should be inside the same #ifdef as their callers d) there's some bug in the script I think for a good number of these modules I've already submitted patches for the unused methods which'll hopefully work their way into the DEV300 line over the next few weeks (and mba has a workspace which should take care of the majority of the binfilter ones). If I recall correctly, the module with the most unused methods that doesn't have anything in the pipeline to remove them is sc, so there's where the lowest hanging fruit should be. C. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
