Patch: Remove unused code.
Moderator: Committer
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Patch: Remove unused code.
Hello fo-devs,
grepping through the code revealed that the classes store_category_impl and store_tech_impl defined in universe/Tech.cpp are not instanced anywhere. The attached patch removes these classes from the code base.
The patch is released under the GPL 2.0 (or later) license.
Regards
Marcel Metz
grepping through the code revealed that the classes store_category_impl and store_tech_impl defined in universe/Tech.cpp are not instanced anywhere. The attached patch removes these classes from the code base.
The patch is released under the GPL 2.0 (or later) license.
Regards
Marcel Metz
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: Patch: Remove unused code.
Hello fo-devs,
After setting up a clean freeorion build I used callcatcher [1] wrapper instead of the regular gcc/g++ as compiler.
The callcatcher project is a application that was initially developed for finding dead code in the libre office code. After a run it detected serveral unused functions and classes that after inspection weren't in fact used at all. The attached patch removes some of those functions. I may attach more patches after I verified that they aren't called conditionally (platform specific or something like that.
[1] http://www.skynet.ie/~caolan/Packages/callcatcher.html
After setting up a clean freeorion build I used callcatcher [1] wrapper instead of the regular gcc/g++ as compiler.
The callcatcher project is a application that was initially developed for finding dead code in the libre office code. After a run it detected serveral unused functions and classes that after inspection weren't in fact used at all. The attached patch removes some of those functions. I may attach more patches after I verified that they aren't called conditionally (platform specific or something like that.
[1] http://www.skynet.ie/~caolan/Packages/callcatcher.html
Re: Patch: Remove unused code.
i'm not a dev, and i'm not looking at the functions you're removing/streamlining...
but the way i understand it this is a beta/alpha game removing uncalled/unreferenced functions from the source code may remove ideas that were left there for a reason.
sure it may increase compile times and decrease binary size
but why lobatomize a W.I.P.?
personally, with the lag i suffer at the hands of fleets and having recently downloaded of the svn
i'm thinking about trying to figure out how to build a cross platform NATIVE library to handle the fleet linked lists and string names cuz i suspect python is handling everything.
but the way i understand it this is a beta/alpha game removing uncalled/unreferenced functions from the source code may remove ideas that were left there for a reason.
sure it may increase compile times and decrease binary size
but why lobatomize a W.I.P.?
personally, with the lag i suffer at the hands of fleets and having recently downloaded of the svn
i'm thinking about trying to figure out how to build a cross platform NATIVE library to handle the fleet linked lists and string names cuz i suspect python is handling everything.
thanks for a great game.
Starcraft, Syndicate, Populous, Star Control II, Master of Orion, Master of Magic, X-COM UFO Defense, Spacehulk: Vengence of the Blood Angels.
Starcraft, Syndicate, Populous, Star Control II, Master of Orion, Master of Magic, X-COM UFO Defense, Spacehulk: Vengence of the Blood Angels.
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: Patch: Remove unused code.
So you didn't even review the patch and just assume it is bad?ogre wrote:i'm not a dev, and i'm not looking at the functions you're removing/streamlining...
As you said; you're not a developer, so please look up what "dead code" and "code smell" is. A lot of smart people have written something to this topic so I don't think I need to repeat the same arguments.
There is no need for a poor man rcs when the project already uses one. Also the rcs history indicates that those methods were never used since said functions were introduced so there are no feature 'lobotomized' from the application. Also the fo-devs should know best what is needed and what not. It's up to them to apply, request or reject patches.ogre wrote:but the way i understand it this is a beta/alpha game removing uncalled/unreferenced functions from the source code may remove ideas that were left there for a reason.
but why lobatomize a W.I.P.?
I fail to see how this relevant to the topic.ogre wrote: personally, with the lag i suffer at the hands of fleets and having recently downloaded of the svn
i'm thinking about trying to figure out how to build a cross platform NATIVE library to handle the fleet linked lists and string names cuz i suspect python is handling everything.
Re: Patch: Remove unused code.
I'm not sufficiently familiar with that code to know why those unused functions are there, or whether or not we want to have them at hand without going back to get them from a previous revision. Perhaps Geoff could comment on that...? Also, a destructor for TechTreeLayout::Edge is among the removed methods, which is probably at least being called when local Edge variables go out of scope... or does the tool you used check for that as well? In which case the fact that that code isn't getting called might be an indication of a memory leak of some sort, though its impossible to say without taking a closer look.
Warning: Antarans in dimensional portal are closer than they appear.
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: Patch: Remove unused code.
I would like to add some facts about that:Bigjoe5 wrote:I'm not sufficiently familiar with that code to know why those unused functions are there, or whether or not we want to have them at hand without going back to get them from a previous revision. Perhaps Geoff could comment on that...?
GetNumberOfChildren:
GetNumberOfParents:
IsFinalNode:
Added: r3960 -- Removed OGDF code to replace with Lathanda's not-quite-finished but functional custom graph layout
Date: Sat Jan 29 23:49:45 2011 +0000
Never referenced anywhere else.
Changed 2 times for code style changes.
IsPlaceHolder:
Added: r3960 -- Removed OGDF code to replace with Lathanda's not-quite-finished but functional custom graph layout
Date: Sat Jan 29 23:49:45 2011 +0000
Referenced 2 times, references where deleted at
5410 -- Tweaks and grooming in tech tree layout code
Date: Tue Nov 13 04:15:27 2012 +000
callcatcher hooks into the compile process, collects all symbols in the object files and compares those collections with the actual linked symbols. If a symbol is not linked (and therefor not used) callcatcher prints it's signature.Bigjoe5 wrote:Also, a destructor for TechTreeLayout::Edge is among the removed methods, which is probably at least being called when local Edge variables go out of scope... or does the tool you used check for that as well?
I would assume that a destructor isn't needed at all in this case because the default destructor handles this case just fine.Bigjoe5 wrote:In which case the fact that that code isn't getting called might be an indication of a memory leak of some sort, though its impossible to say without taking a closer look.
Also it slipped my mind last time to mention that the patch in comment #2 is released under the GPL 2.0 (or later) license.
- Geoff the Medio
- Programming, Design, Admin
- Posts: 13587
- Joined: Wed Oct 08, 2003 1:33 am
- Location: Munich
Re: Patch: Remove unused code.
The functions removed by the second patch are all trivial getters, and the destructor. I find I strange that the latter isn't used... For the former, if they were substantial blocks of dead code, I'd remove them as was the case for the tech store impl functions, but for such small bits as this, I'd rather leave them in case someone else wants to modify this code and might find them useful.
Re: Patch: Remove unused code.
True. Actually, I'd say there are a lot of destructors in the the code that aren't needed, since many classes (most notably in the UniverseObject code I've been looking at) define empty destructors for apparently no reason. I'm also not too sure why these classes have copy constructors, either.adrian_broher wrote:I would assume that a destructor isn't needed at all in this case because the default destructor handles this case just fine.
Warning: Antarans in dimensional portal are closer than they appear.
- Geoff the Medio
- Programming, Design, Admin
- Posts: 13587
- Joined: Wed Oct 08, 2003 1:33 am
- Location: Munich
Re: Patch: Remove unused code.
Are you talking about class UniverseObject and its derived classes? The only class in that hierarchy that has a destructor declared is UniverseObject, the base class, which as far as I know needs to have a virtual destructor for polymorphic delete to work properly.Bigjoe5 wrote:...many classes (most notably in the UniverseObject code I've been looking at) define empty destructors for apparently no reason. I'm also not too sure why these classes have copy constructors, either.
I'm not sure why UniverseObject and System have copy constructors. There is also an "owners" parameters in UniverseObject that's probably a pointless relic of old code. I'll try removing some of that.
Re: Patch: Remove unused code.
That is correct.Geoff the Medio wrote:The only class in that hierarchy that has a destructor declared is UniverseObject, the base class, which as far as I know needs to have a virtual destructor for polymorphic delete to work properly.
https://github.com/mmoderau
[...] for Man has earned his right to hold this planet against all comers, by virtue of occasionally producing someone totally batshit insane. - Randall Munroe, title text to xkcd #556
[...] for Man has earned his right to hold this planet against all comers, by virtue of occasionally producing someone totally batshit insane. - Randall Munroe, title text to xkcd #556
Re: Patch: Remove unused code.
Also ResourceCenter and PopCenter. A quick google search shows that you are correct about that - I have much yet to learn, it seems.Geoff the Medio wrote:Are you talking about class UniverseObject and its derived classes? The only class in that hierarchy that has a destructor declared is UniverseObject, the base class, which as far as I know needs to have a virtual destructor for polymorphic delete to work properly.Bigjoe5 wrote:...many classes (most notably in the UniverseObject code I've been looking at) define empty destructors for apparently no reason. I'm also not too sure why these classes have copy constructors, either.
Warning: Antarans in dimensional portal are closer than they appear.
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: Patch: Remove unused code.
I will abandon the patch in that case. However the destructor should be removed as it serves no purpose.Geoff the Medio wrote:[…]but for such small bits as this, I'd rather leave them in case someone else wants to modify this code and might find them useful.
I've attached some patches under GPL 2.0 or later license. These patches remove the unused TreeLoader2D and GrassLoader class. tzlaine added those as a part of adding the PagedGeometry Ogre library but those classes were never used in the project history.
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: Patch: Remove unused code.
Bigjoe5 wrote:Also, a destructor for TechTreeLayout::Edge is among the removed methods, which is probably at least being called when local Edge variables go out of scope...
You're both right as it seems. The is an dynamic instantiation of Edge on UI/TechTreeLayout.cpp:636 and those are stored in m_out_edges. The pointer aren't freed at all in ~Node (UI/TechTreeLayout.cpp:408.Geoff the Medio wrote:I find I strange that the latter isn't used...