Odd code of the day

Development directions, tasks, and features being actively implemented or pursued by the development team.

Odd code of the day

Postby breese » Tue Dec 14, 2010 11:42 am

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
Topics: 14
Joined: Thu Sep 02, 2010 1:00 pm

Share On:

Share on Facebook Facebook Share on Twitter Twitter Share on Digg Digg

Re: Odd code of the day

Postby breese » Sun Dec 19, 2010 10:49 am

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.
breese
Bounty Hunter
Bounty Hunter
 
Posts: 152
Topics: 14
Joined: Thu Sep 02, 2010 1:00 pm

Re: Odd code of the day

Postby klauss » Sun Dec 19, 2010 11:08 am

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
User avatar
klauss
Elite
Elite
 
Posts: 7243
Topics: 55
Joined: Mon Apr 18, 2005 7:40 am
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Postby breese » Sun Dec 19, 2010 1:02 pm

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.
breese
Bounty Hunter
Bounty Hunter
 
Posts: 152
Topics: 14
Joined: Thu Sep 02, 2010 1:00 pm

Re: Odd code of the day

Postby safemode » Mon Jan 31, 2011 2:56 pm

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.
safemode
Developer
Developer
 
Posts: 2150
Topics: 84
Joined: Sun Apr 22, 2007 6:17 pm
Location: Pennsylvania

Re: Odd code of the day

Postby abee » Fri May 13, 2011 5:37 am

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;
    }
}
abee
Atmospheric Pilot
Atmospheric Pilot
 
Posts: 1
Joined: Fri May 13, 2011 5:31 am

Re: Odd code of the day

Postby AVBJim » Tue May 17, 2011 8:22 am

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.
AVBJim
Merchant
Merchant
 
Posts: 42
Topics: 10
Joined: Tue Dec 22, 2009 2:16 pm

Re: Odd code of the day

Postby strook » Tue May 17, 2011 9:10 am

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
User avatar
strook
ISO Party Member
ISO Party Member
 
Posts: 461
Topics: 20
Joined: Fri Sep 03, 2010 5:10 am

Re: Odd code of the day

Postby loki1950 » Tue May 17, 2011 9:31 am

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 18/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 DELL E6400 4GB ram/100 GB HD Mint 17.3 6
User avatar
loki1950
The Shepherd
 
Posts: 5701
Topics: 51
Joined: Fri May 13, 2005 1:37 pm
Location: Ottawa

Re: Odd code of the day

Postby AVBJim » Sat Jul 23, 2011 8:18 am

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
Topics: 10
Joined: Tue Dec 22, 2009 2:16 pm

Re: Odd code of the day

Postby AVBJim » Sat Jul 23, 2011 8:52 am

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.
AVBJim
Merchant
Merchant
 
Posts: 42
Topics: 10
Joined: Tue Dec 22, 2009 2:16 pm

Re: Odd code of the day

Postby pheonixstorm » Sun Aug 07, 2011 2:06 pm

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
User avatar
pheonixstorm
Elite
Elite
 
Posts: 1567
Topics: 113
Joined: Mon Jan 25, 2010 7:03 pm

src/cmd/unit_generic.cpp

Postby TBeholder » Wed Dec 07, 2011 2:49 pm

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
User avatar
TBeholder
Elite Venturer
Elite Venturer
 
Posts: 732
Topics: 38
Joined: Fri Apr 14, 2006 7:40 pm
Location: chthonic safety

Re: Odd code of the day

Postby klauss » Wed Dec 07, 2011 3:02 pm

I don't get the issue.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
User avatar
klauss
Elite
Elite
 
Posts: 7243
Topics: 55
Joined: Mon Apr 18, 2005 7:40 am
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Postby TBeholder » Thu Dec 08, 2011 4:16 am

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
User avatar
TBeholder
Elite Venturer
Elite Venturer
 
Posts: 732
Topics: 38
Joined: Fri Apr 14, 2006 7:40 pm
Location: chthonic safety

Re: Odd code of the day

Postby klauss » Thu Dec 08, 2011 4:40 pm

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
User avatar
klauss
Elite
Elite
 
Posts: 7243
Topics: 55
Joined: Mon Apr 18, 2005 7:40 am
Location: LS87, Buenos Aires, República Argentina

Re: Odd code of the day

Postby travists » Fri Dec 09, 2011 11:38 am

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.
User avatar
travists
Expert Mercenary
Expert Mercenary
 
Posts: 893
Topics: 33
Joined: Thu Jul 08, 2010 4:43 pm
Location: Sol III North American Continent

Re: Odd code of the day

Postby TBeholder » Sun Dec 11, 2011 3:36 pm

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
User avatar
TBeholder
Elite Venturer
Elite Venturer
 
Posts: 732
Topics: 38
Joined: Fri Apr 14, 2006 7:40 pm
Location: chthonic safety

Re: Odd code of the day

Postby travists » Sun Dec 11, 2011 11:33 pm

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
User avatar
travists
Expert Mercenary
Expert Mercenary
 
Posts: 893
Topics: 33
Joined: Thu Jul 08, 2010 4:43 pm
Location: Sol III North American Continent

Re: Odd code of the day

Postby pheonixstorm » Thu Mar 15, 2012 10:47 pm

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
User avatar
pheonixstorm
Elite
Elite
 
Posts: 1567
Topics: 113
Joined: Mon Jan 25, 2010 7:03 pm

Re: Odd code of the day

Postby pheonixstorm » Thu Mar 15, 2012 11:53 pm

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
User avatar
pheonixstorm
Elite
Elite
 
Posts: 1567
Topics: 113
Joined: Mon Jan 25, 2010 7:03 pm

Re: Odd code of the day

Postby klauss » Fri Mar 16, 2012 9:05 am

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
User avatar
klauss
Elite
Elite
 
Posts: 7243
Topics: 55
Joined: Mon Apr 18, 2005 7:40 am
Location: LS87, Buenos Aires, República Argentina



Return to Engine Development

Who is online

Users browsing this forum: No registered users and 1 guest

cron