Patch: Remove unused code.

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Patch: Remove unused code.

#1 Post by adrian_broher »

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
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Patch: Remove unused code.

#2 Post by adrian_broher »

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

ogre
Space Squid
Posts: 70
Joined: Sun Feb 10, 2013 5:38 am
Location: Flint, Wishagain

Re: Patch: Remove unused code.

#3 Post by ogre »

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

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Patch: Remove unused code.

#4 Post by adrian_broher »

ogre wrote:i'm not a dev, and i'm not looking at the functions you're removing/streamlining...
So you didn't even review the patch and just assume it is bad?
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.
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.?
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: 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.
I fail to see how this relevant to the topic.

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: Patch: Remove unused code.

#5 Post by Bigjoe5 »

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.

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Patch: Remove unused code.

#6 Post by adrian_broher »

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...?
I would like to add some facts about 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

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?
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: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.
I would assume that a destructor isn't needed at all in this case because the default destructor handles this case just fine.

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.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Patch: Remove unused code.

#7 Post by Geoff the Medio »

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.

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: Patch: Remove unused code.

#8 Post by Bigjoe5 »

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.
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.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Patch: Remove unused code.

#9 Post by Geoff the Medio »

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

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.

User avatar
em3
Vacuum Dragon
Posts: 630
Joined: Sun Sep 25, 2011 2:51 pm

Re: Patch: Remove unused code.

#10 Post by em3 »

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.
That is correct.
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

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: Patch: Remove unused code.

#11 Post by Bigjoe5 »

Geoff the Medio wrote:
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.
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.
Also ResourceCenter and PopCenter. A quick google search shows that you are correct about that - I have much yet to learn, it seems.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Patch: Remove unused code.

#12 Post by adrian_broher »

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 will abandon the patch in that case. However the destructor should be removed as it serves no purpose.

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.

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Patch: Remove unused code.

#13 Post by adrian_broher »

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...
Geoff the Medio wrote:I find I strange that the latter isn't used...
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.

Post Reply