Odd code of the day

Development directions, tasks, and features being actively implemented or pursued by the development team.
Post Reply
breese
Bounty Hunter
Bounty Hunter
Posts: 152
Joined: Thu Sep 02, 2010 8:00 pm

Odd code of the day

Post by breese »

Hardly a day goes by without encountering some strange code in VS, so I thought I'd create a thread were you can post your favourite oddities.

Today I came across this conditional statement in UnitCollection::SetUnit() in src/cmd/collection.cpp

Code: Select all

    //if the unit is null then go here otherwise if the unit is killed then go here
    if (un != NULL ? un->Killed() == true : true) {
I would probably have written it like this instead:

Code: Select all

    if ((un == NULL) || un->Killed()) {
breese
Bounty Hunter
Bounty Hunter
Posts: 152
Joined: Thu Sep 02, 2010 8:00 pm

Re: Odd code of the day

Post by breese »

From src/cmd/collide_map.h (lots of lines removed to highlight the odd code)

Code: Select all

class CollideArray
{
public:
    typedef Collidable              *iterator;
    typedef std::vector< Collidable >ResizableArray;
    ResizableArray sorted;
    iterator begin()
    {
        return sorted.size() != 0 ? &*sorted.begin() : NULL;
    }
    iterator end()
    {
        return this->begin()+sorted.size();
    }
From a cursory reading it looks like CollideArray implements the well-known STL iterator idiom with begin() and end() and all.

But it is a fake. The "iterator" is simply a pointer, and begin() and end() are adaptors to avoid sorted's real iterator of type std::vector::iterator.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Post by klauss »

A pointer isn't a "fake" iterator, in fact the whole point of the iterator interface is to mimic pointers.

Now, the way in which that iterator is obtained in begin() truly is awkward... and probably rather bug-prone.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
breese
Bounty Hunter
Bounty Hunter
Posts: 152
Joined: Thu Sep 02, 2010 8:00 pm

Re: Odd code of the day

Post by breese »

klauss wrote:A pointer isn't a "fake" iterator, in fact the whole point of the iterator interface is to mimic pointers.
I did not say that pointers are fake iterators.

Instead, the problem is that CollideArray attempts to mimic the STL iterator idiom (e.g. an STL-like container, with iterator, begin() and end()), but it fails in this regard.

For STL containers, end() will return a sentinel that somehow indicates the end of the container, and begin() will return end() if the container is empty, so that begin() == end(). Now look at CollideArray. begin() returns NULL rather than end() when the array is empty, and end() returns a pointer to the sentinel. This means that begin() != end() when the array is empty. I am not saying that the code cannot be fixed; just that the it is odd.

Furthermore, begin() and end() are used incorrectly by CollideArray to check if an iterator is valid (see CollideArray::Iterable() for an example). Pointer comparison is extremely tricky business; relational pointer comparisons are not guaranteed to work if the pointer is pointing to something outside the container. See paragraph 5.9 [expr.rel] in the C++ standard for the pointer comparison rules. Now, on this account I am willing to admit that the C++ standard is odd (although in its defense, it inherited these rules from the C standard.)
klauss wrote:Now, the way in which that iterator is obtained in begin() truly is awkward... and probably rather bug-prone.
Indeed. That is what triggered me in the first place.
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Odd code of the day

Post by safemode »

What you are seeing is probably the result of how collection used to be a double linked list rather than an STL container. See old_collection.cpp for the legacy implementation. The classes still in VS that built on top of the old collection class tried to pretend it was a container rather than a regular linked list. When i made the new collection class a long time ago i made it as drop-in friendly as possible so nearly no changes needed to be made and no real behavior was altered. Or as little as possible to minimize the changeset and allow us to see how it performed against the old one. Re-implementation of these higher level classes would probably be good.
Ed Sweetman endorses this message.
abee
Atmospheric Pilot
Atmospheric Pilot
Posts: 1
Joined: Fri May 13, 2011 12:31 pm

Re: Odd code of the day

Post by abee »

Is there a reason why ignore_network is checked twice?

Code: Select all

if (!ignore_network) {
    cout<<"Number of local players = "<<numplayers<<endl;
    //Initiate the network if in networking play mode for each local player
    if (!ignore_network) {
        setNewTime( 0. );
        //Get the number of local players
        Network = new NetClient[numplayers];
        //Here we say we want to only handle activity in 1 starsystem not more
        run_only_player_starsystem = true;
    } else {
         Network = NULL;
         cout<<"Non-networking mode"<<endl;
         //Here we say we want to only handle activity in 1 starsystem not more
         run_only_player_starsystem = true;
    }
}
AVBJim
Merchant
Merchant
Posts: 42
Joined: Tue Dec 22, 2009 9:16 pm

Re: Odd code of the day

Post by AVBJim »

Ok, I am new to C++ programming and still learning. The way I read this, and I could be wrong, but the braces force the else statement to match the inner if. Because the inner if and outer if are the same, the second (inner if) will alway evaluate as true and the else function will never be used.

Here are my possible fixes.

Code: Select all

//This solution eliminates repetitive if statements and combines them. cout from the outer is moved into inner if and outer if removed
 //Initiate the network if in networking play mode for each local player
if (!ignore_network) {
        cout<<"Number of local players = "<<numplayers<<endl;
        setNewTime( 0. );
        //Get the number of local players
        Network = new NetClient[numplayers];
        //Here we say we want to only handle activity in 1 starsystem not more
        run_only_player_starsystem = true;
    } 
else {
         Network = NULL;
         cout<<"Non-networking mode"<<endl;
         //Here we say we want to only handle activity in 1 starsystem not more
         run_only_player_starsystem = true;
    }
You could also move the else so that the brackets force it to the outer if statement. To me though, having a nested if that does the same check as the original if just seems wrong somehow. I would appreciate any feed back, as I stated, I am still learning.
strook
ISO Party Member
ISO Party Member
Posts: 461
Joined: Fri Sep 03, 2010 12:10 pm

Re: Odd code of the day

Post by strook »

Code: Select all

if(a == true) 
{
if ...
else //if a == false
{}
}
the else section will in fact never reaced, you are right.
plz visit my vegastrike project branch here

plz support VegaOgre by donating to it!

My systems: Mac mini 1, 4gig RAM;
i5 Quad Core 2400, 300mbit WLAN, 1,3Tbyte HD, 60 GB SSD,
nvidia geforce 8400gs 512MB, 6gig RAM with Ubuntu 11.4,
win7 and hackintosh installed
loki1950
The Shepherd
Posts: 5841
Joined: Fri May 13, 2005 8:37 pm
Location: Ottawa
Contact:

Re: Odd code of the day

Post by loki1950 »

Just a comment from the peanut gallery been awhile since I've been around and I am catching up. But this is a thread that should have created years ago :mrgreen:

Enjoy the Choice :)
my box::HP Envy i5-6400 @2Q70GHzx4 8 Gb ram/1 Tb(Win10 64)/3 Tb Mint 19.2/GTX745 4Gb acer S243HL K222HQL
Q8200/Asus P5QDLX/8 Gb ram/WD 2Tb 2-500 G HD/GF GT640 2Gb Mint 17.3 64 bit Win 10 32 bit acer and Lenovo ideapad 320-15ARB Win 10/Mint 19.2
AVBJim
Merchant
Merchant
Posts: 42
Joined: Tue Dec 22, 2009 9:16 pm

Re: Odd code of the day

Post by AVBJim »

In file ../vegastrike/vegastrike/src/SharedPool.h

The file calls #include "gnuhash.h" however, other files use <gnuhash.h> would this cause the calls to gnuhash.h to be invalid if the wrong include is used? A search of my filesystem tuns up gnuhash.h as being in the ../vegastrike/vegastrike/src folder, should all calls to it be "gnuhash.h" and not <gnuhash.h>?

Also the file calls #include "SharedPool.cpp". SharedPool.cpp calls #include SharedPool.h. What effect does this looping type call have on the actual compiled files?
AVBJim
Merchant
Merchant
Posts: 42
Joined: Tue Dec 22, 2009 9:16 pm

Re: Odd code of the day

Post by AVBJim »

File images.h in the /vegastrike/vegastrike/src/cmd folder
#include "../SharedPool.h"
I have only seen this type of include a couple of times. Is this a proper include? The ../ is to indicate that the directory should be moved back, at least I think that is what is indicates. Looking through other files, they do not include the ../ when calling files from parent directories.
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Odd code of the day

Post by pheonixstorm »

Anything is possible with this code... though i have seen somefile.h inclue somefile.cpp which had an include somefile.h :roll: If i remember correctly there were a few like this that were a real bitch to fix...

Also, one of the include files (i dont remember which) will not work right if you add inclusion guards.

So no

Code: Select all

#ifndef BLAH_H
#define BLAH_H
lest you get a few dozen compile errors.
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
TBeholder
Elite Venturer
Elite Venturer
Posts: 753
Joined: Sat Apr 15, 2006 2:40 am
Location: chthonic safety

src/cmd/unit_generic.cpp

Post by TBeholder »

Vector Unit::ClampTorque

Code: Select all

    static float Lithium6constant =
        XMLSupport::parse_float( vs_config->getVariable( "physics", "LithiumRelativeEfficiency_Lithium", "1" ) );
    //1/5,000,000 m/s
    static float FMEC_exit_vel_inverse =
        XMLSupport::parse_float( vs_config->getVariable( "physics", "FMEC_exit_vel", "0.0000002" ) );
    //HACK this forces the reaction to be Li-6+D fusion with efficiency governed by the getFuelUsage function
    fuel -= GetFuelUsage( false )*SIMULATION_ATOM*Res.Magnitude()*FMEC_exit_vel_inverse/Lithium6constant;
Vector Unit::ClampThrust

Code: Select all

    static float Lithium6constant =
        XMLSupport::parse_float( vs_config->getVariable( "physics", "DeuteriumRelativeEfficiency_Lithium", "1" ) );
    //1/5,000,000 m/s
    static float FMEC_exit_vel_inverse =
        XMLSupport::parse_float( vs_config->getVariable( "physics", "FMEC_exit_vel", "0.0000002" ) );
[...]
    if (3 == afterburntype || afterburntype == 1) {
        //fuel-burning overdrive - uses afterburner efficiency. In NO_AFTERBURNER case, "afterburn" will always be false, so can reuse code.
        //HACK this forces the reaction to be Li-6+Li-6 fusion with efficiency governed by the getFuelUsage function
        fuel -=
            ( (afterburn
               && finegrainedFuelEfficiency) ? afterburnenergy : GetFuelUsage( afterburn ) )*SIMULATION_ATOM*Res.Magnitude()
            *FMEC_exit_vel_inverse/Lithium6constant;
So, 2 same-but-different constant for turning and linear acceleration (including maneuvering).
Then for two most frequently used variants (fuel-guzzler and no afterburner) reusing code with extra juggling instead of a clear switch-case.
"Two Eyes Good, Eleven Eyes Better." -Michele Carter
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Post by klauss »

I don't get the issue.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
TBeholder
Elite Venturer
Elite Venturer
Posts: 753
Joined: Sat Apr 15, 2006 2:40 am
Location: chthonic safety

Re: Odd code of the day

Post by TBeholder »

klauss wrote:I don't get the issue.
one fuel constant for turning, another for straight acceleration. The rest is not bad, but unnecessary clutter.
"Two Eyes Good, Eleven Eyes Better." -Michele Carter
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Post by klauss »

It seems to be on purpose. One is li6+li6 and the other is li6+deuterium.

At least that's what I gather from the comments.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
travists
Expert Mercenary
Expert Mercenary
Posts: 893
Joined: Thu Jul 08, 2010 11:43 pm
Location: Sol III North American Continent

Re: Odd code of the day

Post by travists »

I shall leave the code talk to the more experienced programers. However, in looking at this thread I think it is good for far more then just a good belly laugh at how odd some of the code is. As strange code is found it can more easily be brought to light, and if the conclusion is "What where they/we thinking?" it can be scrapped or re-written. Similarly, "There must be a better way of doing this!" can streamline the code and "Oh, that's why its done like this." can add comments to clear it up. After all, sloppy code does not always cause bugs that are likely to show up, but it can increase memory requirements and slow things down.
TBeholder
Elite Venturer
Elite Venturer
Posts: 753
Joined: Sat Apr 15, 2006 2:40 am
Location: chthonic safety

Re: Odd code of the day

Post by TBeholder »

klauss wrote:It seems to be on purpose. One is li6+li6 and the other is li6+deuterium.
At least that's what I gather from the comments.
Yep. Deuterium fuel, which is not counted or implemented in any other way and used for side thrust (which BTW also uses afterburner) but not turning. That's odd, isn't? :wink:
"Two Eyes Good, Eleven Eyes Better." -Michele Carter
travists
Expert Mercenary
Expert Mercenary
Posts: 893
Joined: Thu Jul 08, 2010 11:43 pm
Location: Sol III North American Continent

Re: Odd code of the day

Post by travists »

Deuterium is good for fusion, but my take is use it or loose it. All fuel is consumable and should be tracked. Further, if "deuterium" is implied to be anything other than Hydrogen with an extra neutron change the name. It is a commonly known chemical name, and if used as anything other than what it is could cause confusion.

And for the record li-6 is real and has nuclear applications
http://www.fas.org/nuke/intro/nuke/lithium.htm
http://en.wikipedia.org/wiki/Isotopes_of_lithium
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Odd code of the day

Post by pheonixstorm »

Found this dead code section while cleaning things up a bit... planet_generic.cpp

Code: Select all

void Planet::gravitate( UnitCollection *uc )
{
    /*
     *  float *t = cumulative_transformation_matrix;
     *
     *
     *  if(gravity!=0.0&&uc) {
     *  Iterator *iterator = uc->createIterator();
     *  Unit *unit;
     *  Vector vec(0,0,0);
     *
     *  while((unit = iterator->current())!=NULL) {
     *   if(unit->type()!=PLANETPTR) {
     *     Vector r = (unit->Position() - (vec.Transform(t)));
     *     //      cerr << "Unit (" << unit << "): " << endl;
     *     //      cerr << "Gravity source: " << vec.Transform(t) << "\nUnit position: " << unit->Position() << "\nDelta: " << r << endl;
     *     float _r_ = r.Magnitude();
     *     r = r * (1.0/_r_);
     *     r =  r * -(gravity/(_r_*_r_));
     *     //      cerr << "Distance: " << _r_ << "\nGravity force vector: " << r << endl;
     *
     *     if(_r_ > radius) {
     *       unit->Accelerate(r);
     *     }
     *   }
     *   iterator->advance();
     *  }
     *  delete iterator;
     *  }*/

    //fake gravity
    /***FIXME 091401 why do we need to traverse satellites??????
     *  un_iter * iter;
     *  for (iter = satellites.createIterator();
     *    iter->current()!=NULL;
     *    iter->advance()) {
     *     if (iter->current()->isUnit()==PLANETPTR)
     *         ((Planet *)iter->current())->gravitate(uc);//FIXME 071201
     *     else { //FIXME...[causes flickering for crashing orbiting units
     *
     *       //((Unit *)iter->current())->ResolveForces (identity_transformation,identity_matrix,false);
     *       ((Unit *)iter->current())->UpdateCollideQueue();
     *     }
     *  }
     *  delete iter;
     **/
}
This entire block has been commented out since before r500! No to mention this section in planet.cpp that calls it.

Code: Select all

void GamePlanet::gravitate( UnitCollection *uc )
{
    //Should put computation only in Planet and GFX/SFX only here if needed
    Planet::gravitate( uc );
}
Not even sure how long this ugly thing has been floating around calling nothing...
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Odd code of the day

Post by pheonixstorm »

Here's another odd duck. unit_generic.cpp

Code: Select all

#ifdef ISUCK
            Destroy();
#endif
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Post by klauss »

pheonixstorm wrote:Here's another odd duck. unit_generic.cpp

Code: Select all

#ifdef ISUCK
            Destroy();
#endif
That's perfectly reasonable. Anyone that sucks should be destroyed. :P
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
Post Reply