Code reformatting project [DONE]

Development directions, tasks, and features being actively implemented or pursued by the development team.
Post Reply
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

klauss wrote:
chuck_starchaser wrote:Man! This is so hard!
The cost of bad design.
The Mother of Understatements... This is such spaghetti!

Code: Select all

void Unit::Init()
{
        pImage->cockpit_damage = (float*) malloc( (numg)*sizeof (float) );
Other pointer members in UnitImages use new and delete; but cockpit_damage uses malloc and free.

Unit has a UnitImages *pImage (I renamed it to pImage from just "image"), but the member is public,
like most data members everywhere, and allocation and deallocation are left to clients to manage.

Now, from Unit::Init()...

Code: Select all

    pImage->next_repair_time  = -FLT_MAX;
    pImage->next_repair_cargo = ~0;
    pImage->ecm = 0;
    pImage->cloakglass        = false;
    pImage->CargoVolume       = 0;
    pImage->UpgradeVolume     = 0;
    pImage->unitwriter        = NULL;
    cloakmin = pImage->cloakglass ? 1 : 0;
    pImage->equipment_volume  = 0;
    pImage->HiddenCargoVolume = 0;
    pImage->cloakrate = 100;
    pImage->cloakenergy       = 0;
    pImage->forcejump = false;
    ..................
    pImage->fireControlFunctionality    = 1.0f;
    pImage->fireControlFunctionalityMax = 1.0f;
    pImage->SPECDriveFunctionality      = 1.0f;
    pImage->SPECDriveFunctionalityMax   = 1.0f;
    pImage->CommFunctionality           = 1.0f;
    pImage->CommFunctionalityMax        = 1.0f;
    pImage->LifeSupportFunctionality    = 1.0f;
    pImage->LifeSupportFunctionalityMax = 1.0f;
what have ANY of these parameters got to do with images?????
Like, do these Berkeley people have the slightest concept of using proper names for things?
I have NO IDEA what the purpose of UnitImages is. None whatsoever. All I'm going by is the
theory that it can't have been used by vegaserver, otherwise it would have needed
to include gfx/sprite.cpp in the build list, which it didn't.

Excerpt from Unit::UpdatePhysics()...

Code: Select all

        ...................................
            SetShieldZero( this );
            if (pImage->cloakrate > 0 || cloaking == cloakmin) {
                if (warp_energy_for_cloak)
                    warpenergy -= (SIMULATION_ATOM*pImage->cloakenergy);
                else
                    energy -= (SIMULATION_ATOM*pImage->cloakenergy);
            }
            if (cloaking > cloakmin) {
                AUDAdjustSound( sound->cloak, cumulative_transformation.position, cumulative_velocity );
                //short fix
                if ( (cloaking == (2147483647) && pImage->cloakrate > 0) || (cloaking == cloakmin+1 && pImage->cloakrate < 0) )
                    AUDStartPlaying( sound->cloak );
                //short fix
                cloaking -= (int) (pImage->cloakrate*SIMULATION_ATOM);
                if (cloaking <= cloakmin && pImage->cloakrate > 0)
                    //AUDStopPlaying (sound->cloak);
                    cloaking = cloakmin;
                if (cloaking < 0 && pImage->cloakrate < 0) { ..........
See that pImage->cloakenergy thing being multiplied by SIMULATION_ATOM (probably a macro)?
So, I can't get rid of a dependency to VSSprite without breaking the physics... :( :( :(
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

Oh man; I spent hours on an experiment, and it didn't work. I made UnitImages a bogus template
class, in the hope that the linker would weed out anything not used, and break that VSSprite
dependency; had to replace gazillions of instances of UnitImages with UnitImages<void>; and
once I finally got to the linker again, same story as before...

Code: Select all

src/cmd/ai/aggressive.o: In function `~UnitImages':
/home/d/vs/vegastrike/./src/cmd/images.h:215: undefined reference to `VSSprite::~VSSprite()'
/home/d/vs/vegastrike/./src/cmd/images.h:215: undefined reference to `VSSprite::~VSSprite()'
/home/d/vs/vegastrike/./src/cmd/images.h:215: undefined reference to `VSSprite::~VSSprite()'
src/universe_generic.o: In function `Universe::LoadFactionXML(char const*)':
universe_generic.cpp:(.text._ZN8Universe14LoadFactionXMLEPKc[Universe::LoadFactionXML(char const*)]+0x22): undefined reference to `Faction::LoadXML(char const*, char*, int)'
src/gfx/mesh.o: In function `Mesh':
/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
collect2: ld returned 1 exit status
make: *** [vegaserver] Error 1
So, I guess I'm just going to have to finish the unit_generic_server.cpp file I started... :(
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

Any reason why the physics and graphics got smashed together like that? The server should need 0.0 graphics functions. All it needs to know is who is where and relay basic ship traffic and combat status. The client should handle all the graphics calls based on supplied coord data and collision data. Although the server was an afterthought wasnt it... :evil:
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

pheonixstorm wrote:Although the server was an afterthought wasnt it... :evil:
Exactly. But, you see, proper mental modeling of the problem would have resulted in well defined
client and server. Why? That's what Object Oriented design is all about: each class is a kind of server
to its clients. Applying this philosophy at all levels would have resulted in code that already has well
defined server and client, even if the serving is local and immediate; and would have been relatively
easy to multithread, and to scale to network/internet topologies. You try to minimize dependencies,
serialize data flows, etceteras, and you end up with something nice and modular and easy to work
with and understand.
Instead, they coded an incomprehensible rats' nest, probably trying to "save time" at each step, like
making data members of classes public, to avoid the trouble of writing a get function... then they
spent like three years working on the multiplayer aspect and never got it to work...
Nothing can be easily added or to, or fixed, in this mess, without refactoring the crap out of it first.
Heck; it can't even be refactored before it's cleaned up, like making data members private, first of
all, naming classes and files in a way that says what they are. Right now it's too messy to refactor.
Needs to be made more clear first. Needs to have casts removed. Needs to have pointers replaced
by std::auto_ptr<>'s, C strings converted to std::string<>'s, mallocs and frees replaced by new and
delete's, etceteras. Most functions have parameter lists full of ( int, int, int, float, int, float... ) like
everything, from factions to unit types to everything are specified by raw ints, instead of types. It
is so easy to pass a wrong parameter to a function say that accepts a faction as an int, and pass to
it an image format number instead. ALL parameters in a program should be named types. And so on
and so forth. This code is so bad it should be made illegal.
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

Makes me wonder how the new project they are working on will look
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

I wonder that, too.

Well, good news: I got rid of the VSSprite dependency. My linker output now is,

Code: Select all

universe_generic.cpp:(.text._ZN8Universe14LoadFactionXMLEPKc[Universe::LoadFactionXML(char const*)]+0x22): undefined reference to `Faction::LoadXML(char const*, char*, int)'
src/gfx/mesh.o: In function `Mesh':
/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
collect2: ld returned 1 exit status
make: *** [vegaserver] Error 1
Not sure how hard or easy the two missing links remaining will be.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

One missing link to go...

Code: Select all

/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
/home/d/vs/vegastrike/src/gfx/mesh.cpp:241: undefined reference to `Mesh::LoadBinary(char const*, int)'
collect2: ld returned 1 exit status
make: *** [vegaserver] Error 1
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

DONE! :D, and committed: r12633
So, unit.cpp now compiles to a unit.o, for both vegastrike and vegaserver.
The medusa has been mortally wounded; its days are counted.
Built and tested in both debug and release, and vegastrike works, albeit with the
same glitches mentioned earlier; and vegaserver runs, though it complains about a
missing file,

Code: Select all

['/home/d/vs/data/modules/builtin', '/home/d/vs/data/modules/quests', '/home/d/vs/data/modules/missions', '/home/d/vs/data/modules/ai', '/home/d/vs/data/modules', '/home/d/vs/data/bases']
!!! ERROR : opening dynamic universe file dynaverse.dat !!!


 ======== SERVER IS NOW RUNNING ========
    Server Port: 6777
    Server IP Addresses: 
        No network interfaces found associated to your hostname.
        (Consult the '/sbin/ifconfig' command-line tool for your IP.)
        You can also connect locally using 'localhost'
    Private Server
 --------------------------------------- 
To stop this server, hit Ctrl-C, Ctrl-\, Ctrl-Break, or close this window.

Have fun!
Watch the Vegastrike development Timeline on TRAC (updates every 15 minutes)
In fact, it's not TRAC that updates every 15 minutes; it's the wcjunction mirror of vs svn.
And, by the way, it can be used for checkout's, too:
Vegastrike SVN mirror on wcjunction.com /trunk
Username is username; password is password; unless you have registered an account already.

So, I'm getting back to work on fishing out bad macros and wrapping them in do{...}while(0) loops.

EDIT:
Found two huge, naked, multi-line macros in, of all places, irony be me, debug_vs.h.
charlieg
Elite Mercenary
Elite Mercenary
Posts: 1329
Joined: Thu Mar 27, 2003 11:51 pm
Location: Manchester, UK
Contact:

Re: Code reformatting project

Post by charlieg »

Another thing that needs doing is for these classes to get commented with appropriate descriptions so other people don't run into the same 'what the hell is this?' issues.
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

chuck_starchaser wrote:had to replace gazillions of instances of UnitImages with UnitImages<void>
You have to be more lazy! ;)
You could have written:

Code: Select all

#define UnitImages _UnitImages<void>
template<> class _UnitImages { ...
;)
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

@CharlieG: I so wish there was even a little blurb at the top of each file; but I can't write
them; I've got no idea what anything does. Yesterday I was doing some more cleaning of
unit_generic.cpp, where class Unit is defined, and I saw all kinds of code about buying
and selling stuff in there; would have thought that'd be in basecomputer. Maybe the
latter is just about the graphical interface? Who knows...

@Klauss: Yeah, that would at least have made it easier to roll back the change.

Anyhow, I've probably fixed well over a hundred macros by now; yet, no difference
in terms of the vegastrike artifacts.
However, I just decided to test vegaserver again, just in case... And the error
message I used to get about failing to load the universe, is gone :)

Code: Select all

sys.path = [r"/home/d/vs/data/modules/builtin",r"/home/d/vs/data/modules/quests",r"/home/d/vs/data/modules/missions",r"/home/d/vs/data/modules/ai",r"/home/d/vs/data/modules",r"/home/d/vs/data/bases"]
['/usr/lib/python2.4', '/usr/lib/python2.4/plat-linux2', '/usr/lib/python2.4/lib-tk', '/usr/lib/python2.4/lib-dynload', '/usr/local/lib/python2.4/site-packages', '/usr/lib/python2.4/site-packages']
testing VS randomrunning import sys
print sys.path
['/home/d/vs/data/modules/builtin', '/home/d/vs/data/modules/quests', '/home/d/vs/data/modules/missions', '/home/d/vs/data/modules/ai', '/home/d/vs/data/modules', '/home/d/vs/data/bases']
	Exiting ReadSavedPackets


 ======== SERVER IS NOW RUNNING ========
    Server Port: 6777
    Server IP Addresses: 
        No network interfaces found associated to your hostname.
        (Consult the '/sbin/ifconfig' command-line tool for your IP.)
        You can also connect locally using 'localhost'
    Private Server
 --------------------------------------- 
To stop this server, hit Ctrl-C, Ctrl-\, Ctrl-Break, or close this window.

Have fun!
I'm going through one folder at a time, macro fishing. Did the main /src folder, /src/gfx,
/src/cmd/ai, src/cmd/collision2 (lots there... I know it's a library, but heck, I even found
a second bug in it: One macro was broken in half by a blank line.), and I'm about to do
/src/cmd/, followed probably by /src/gldrv, and /src/gui... not sure where to look any
more. But anyhow, I'm going to commit now, so the commits are not so gargantuous...

r12634
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Code reformatting project

Post by safemode »

I really hope i can find some free time to dive into some refactoring. Too much fun to miss out, but my schedule is F'd. Crossing fingers.

edit:

i absolutely hate the GameUnit template setup that we use. In no way shape or form does it make sense to have a polymorphic class on top of a template when we are in control of what is above and below and inside the whole setup.

I'm pretty sure the template exists for python's benefit only. But that should be the end of it. There shouldn't be an even higher level class that inherits this template. Basically, the difference between using something like std::vector<char> and std::vector<int> vs using the imaginary VectorChar and VectorInt which are imaginary polymorphic classes inheriting std::vector<char> and std::vector<int> respectively. Retarded.

Units should be complete at the unit class level. As in, Unit::Planet , should contain all the data and members required to access and set/manipulate them that a Planet unit has. Same goes for bases, asteroids, nebulas, missiles, etc. (ps. ships need their own type, not just called Unit, as that's confusing. A ship should inherit unit like everything else, but a ship has vastly different members than a Planet unit and such).

I can't off the top of my head think of what a template class on top of our polymorphic Unit setup would accomplish other than it being something that python required to reduce python complication.
Ed Sweetman endorses this message.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

@chuck: do you have a log of the macros you've been fixing?

I would like to do the same fixing in trunk, but the svn logs show too many formatting changes and it's difficult to browse. If there's no choice I will, but I thought I'd ask first :)
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Code reformatting project

Post by safemode »

klauss wrote:@chuck: do you have a log of the macros you've been fixing?

I would like to do the same fixing in trunk, but the svn logs show too many formatting changes and it's difficult to browse. If there's no choice I will, but I thought I'd ask first :)

I'd be careful with the macro fixing. It seems to be the cause of the glitches he started seeing at runtime ....of which i believe still persist in his branch. Not saying the fixes aren't correct, but glitches caused by changing things related to the macros indicates that the fixes to them are incomplete still.
Ed Sweetman endorses this message.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

safemode wrote:
klauss wrote:@chuck: do you have a log of the macros you've been fixing?

I would like to do the same fixing in trunk, but the svn logs show too many formatting changes and it's difficult to browse. If there's no choice I will, but I thought I'd ask first :)

I'd be careful with the macro fixing. It seems to be the cause of the glitches he started seeing at runtime ....of which i believe still persist in his branch. Not saying the fixes aren't correct, but glitches caused by changing things related to the macros indicates that the fixes to them are incomplete still.
No; it's the other way around: The glitches are 99.99999% probably because of uncrustify removing
curly braces from around single line conditional blocks. If what looks to uncrustify as a single statement
is actually a macro

Code: Select all

#define BAD_JUJU() \
    run(); \
    stop(); \
    scratch();
, after removing the braces only run() is done conditionally; stopping and scratching get
done unconditionally. The time-honored way to write a command macro is,

Code: Select all

#define OK_JUJU() \
    do { \
        run(); \
        stop(); \
        scratch(); \
    } while(0)
(no semicolon at the end). This makes the macro behave syntactically as
void ok_juju(){...}, in the sense that it will require a semicolon, won't mess up nested conditionals,
you can't assign its value to a variable, you can't assign a value to it, and will not require braces
when it's alone after an if, for or while. There's little or no risk fixing the macros; and much to
gain.

@Klauss: Why you want to fix the macros in trunk? I thought we were going to merge the reformat
branch to trunk tonight! :)
EDIT:
No, but seriously; why don't you check-out reformat and help me fix the two glitches, and let's
get this finished and committed to trunk ASAP, instead of duplicating work.
Last edited by chuck_starchaser on Thu Feb 18, 2010 10:38 pm, edited 1 time in total.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

Actually, wrapping multiline macros in do {} while(0) is common practice, and a good practice. Multiline macros look like single-line statments when instantiated, but act as multiple statements when expanded. That's bad, it creates difficult to track problems.

So I was going to, one by one, fix the macros. Not mindlessly and not automatically.

Even more, some macros could be turned into functions probably.

What broke stuff I think is the formatter, which got confused by multiline macros... In a case like this:

Code: Select all

#define SWAP(x,y,tmp) tmp = x; x = y; y = tmp;
if (a < b) SWAP(a,b,tmp);
I really doubt the intended meaning was what it expands to:

Code: Select all

if (a < b) tmp = x; x = y; y = tmp;
Which is

Code: Select all

if (a < b) 
    tmp = x; 
x = y;
y = tmp;
But rather probably

Code: Select all

if (a < b) { tmp = x; x = y; y = tmp; }
Anyway, wrapping the macro in plain {} doesn't work in every case, what does work in every case is do { ... } while(0)
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

@chuck: the why is... well... remember how the reformat branch was supposed to work.

You have to merge changes from trunk into your branch, and any conflict should be easily resolvable by simply re-running the formatter on trunk.

So trunk has to be formatter-friendly, which is a good thing. And fixing macros is a good thing.

I don't think you should merge the branch into trunk without carefully finding out why the glitches started happening, it could be a mess if you do. From what I understood the glitches were still there, so I definitely wouldn't merge tonight.

And since you started doing other work in the branch than formatting, I believe the other work should be "backported" into trunk first - then the formatter do its thing. And fixing the macros is rather quick ;)

Besides... your working copy is dirty as crazy I'd bet, because of all the trial and error with unit_server.cpp. As a habit of mine which avoids committing messy stuff, I never commit after such a mess of trial and error - I build a patch, visually inspect the patch for "messiness", which can be either untidy stuff or simply wrong or unneeded stuff, and then apply the patch to a clean base. Fixing the macros by hand on trunk is pretty much like doing that without inspecting a patch, and I was going to propose the same with the separation of unit.cpp into unit_server and friends.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

Klauss, as I was just saying, why duplicate work? The macros are done. The only obstacle
with merging is a couple of graphical glitches you could help me out fixing, instead. Then
we add our work-forces, instead of wasting time doing the same thing.
No, but seriously; why don't you check-out reformat and help me fix the two glitches, and
let's get this merged to trunk ASAP.

EDIT:
We posted at the same time.
I was joking about merging tonight. Tomorrow morning is fine. :)
No, seriously; I've no intention of merging WITH glitches; but I'm optimistic about getting
them fixed; and optimistic squared if you help me with it.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

klauss wrote:I don't think you should merge the branch into trunk without carefully finding out why the glitches started happening, it could be a mess if you do. From what I understood the glitches were still there, so I definitely wouldn't merge tonight.
Totally agreed. So, give me a hand with it.
And since you started doing other work in the branch than formatting, I believe the other work should be "backported" into trunk first - then the formatter do its thing. And fixing the macros is rather quick ;)
What?!!!
Are you saying I should remove the reformatting, leaving only the other changes? How would I do that?, and what's the point? Does it matter, historically whether reformatting comes before or after macro fixing or unit refactoring?
It sounds like you're saying that reformatting MUST come AFTER a. Why not then after b, c, ... No matter when
the refactoring goes in, the consequences are the same; and the simplest way to go right now is for it to go
in the order that it happened; --before macro fixing and unit refactoring.
99% of the reformatting was done in two early commits; most of the work since then has been code cleaning,
unit refactoring and macro fixing, and adding const's and explicit's. And when you merge, if my understanding
is correct, the branch commits are not fused together; they show individually. Well, today's commit has some
reformatting because I hadn't touched the collision library, but today I fixed macros in there, so I reformatted
(to MY style, which is pretty close to the original style of the library), --and only the files I worked on.
Besides... your working copy is dirty as crazy I'd bet, because of all the trial and error with unit_server.cpp. As a habit of mine which avoids committing messy stuff, I never commit after such a mess of trial and error - I build a patch, visually inspect the patch for "messiness", which can be either untidy stuff or simply wrong or unneeded stuff, and then apply the patch to a clean base. Fixing the macros by hand on trunk is pretty much like doing that without inspecting a patch, and I was going to propose the same with the separation of unit.cpp into unit_server and friends.
The trial and error was without commits, so no dirt at all. Actually, the only dirt is I forgot to remove the #ifdef VEGASERVER_COMPILING's, but they affect nothing, since the makefile no longer defines it. I'll take them out before the next commit.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

chuck_starchaser wrote:
klauss wrote:I don't think you should merge the branch into trunk without carefully finding out why the glitches started happening, it could be a mess if you do. From what I understood the glitches were still there, so I definitely wouldn't merge tonight.
Totally agreed. So, give me a hand with it.
I'm completely lost with it. When you do small changes and something breaks, you know the problem is there, on those changes. When you batch-process a hundred files and something breaks, all you can hope for is svn revert, you won't find the line that broke it all, not without massive effort.

Hence my idea of going through the non-reformat changes again, in trunk, step by step. They're small, and if they break anything we'll notice right away and probably find out why (before the commit of course).

I've been receiving the diffs of the commit through the vegastrike-cvs hook, I don't know if you're on that mailing list, it receives automatically generated emails with the changes happening on SVN. And, honest, I can't make heads or tails of such huge commits. It's usually not a good idea to mix real changes, real fixes, with reformatting, because the diffs become really hard to inspect. If it's just reformatting you put "only reformatting" on the svn comment and people know what they're looking at. If you put "reformatting + this bug fixed + this refactoring..."... well, it's harder.

Soo... I doubt I can find the glitch no matter how much I try. I've taken shallow looks at the subject and I have no clue, and I suspect I'll remain clueless.
chuck_starchaser wrote:
And since you started doing other work in the branch than formatting, I believe the other work should be "backported" into trunk first - then the formatter do its thing. And fixing the macros is rather quick ;)
What?!!!
Are you saying I should remove the reformatting, leaving only the other changes? How would I do that?
You know what you changed - if you make a list of what macros you fixed, you just list them all and fixing them again is plain simple and fast - the hardest part was finding them.
About unit_server.cpp... that one's harder. If you send a patch (svn diff of your working directory) I could inspect and review it - that's what I was talking about code review. It's nice, because it helps keep the source quality high, and because it's a nice place to exchange ideas and basically learn from each other and make coding decisions - the forum isn't made for that.
I'll see about installing a review board in wcjunction with your permission.
chuck_starchaser wrote:and what's the point?
Keeping things clean and not introducing any new bugs...
chuck_starchaser wrote:Does it matter, historically whether reformatting comes before or after macro fixing or unit refactoring?
...you've experienced it yourself. You HAD to fix macros because they broke reformatting. So reformatting MUST come AFTER fixing the macros, so that reformatting works right - or did I make a mistake somewhere in that reasoning?
If you fix the macros after the formatter made the mistakes, there's no telling what leftover mess could there be.
chuck_starchaser wrote:And when you merge, if my understanding
is correct, the branch commits are not fused together; they show individually.
Ehm... not to my knowledge.
They do get fused together.

"svn merge" is a tool that incorporates the changes from a branch into your working directory, then you commit those changes - just one commit. If you want SVN history to show the commits separately (ie: they're different changesets, different features) you have to merge them step by step.

That's why I support task-oriented branches (one for reformatting and reformatting alone, one for separating unit.cpp into unit_server, etc...), so merging only merges changes of one task, not several.
chuck_starchaser wrote:The trial and error was without commits, so no dirt at all. Actually, the only dirt is I forgot to remove the #ifdef VEGASERVER_COMPILING's, but they affect nothing, since the makefile no longer defines it. I'll take them out before the next commit.
Glad you managed that - I usually inspect the patch just in case, and find one thing or another I had forgotten about. Just do "svn diff | less" and read through, if you want. A quick look is enough to remind you of things you did.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

klauss wrote:
chuck_starchaser wrote:
klauss wrote:I don't think you should merge the branch into trunk without carefully finding out why the glitches started happening, it could be a mess if you do. From what I understood the glitches were still there, so I definitely wouldn't merge tonight.
Totally agreed. So, give me a hand with it.
I'm completely lost with it. When you do small changes and something breaks, you know the problem is there, on those changes. When you batch-process a hundred files and something breaks, all you can hope for is svn revert, you won't find the line that broke it all, not without massive effort.

Hence my idea of going through the non-reformat changes again, in trunk, step by step. They're small, and if they break anything we'll notice right away and probably find out why (before the commit of course).

I've been receiving the diffs of the commit through the vegastrike-cvs hook, I don't know if you're on that mailing list, it receives automatically generated emails with the changes happening on SVN. And, honest, I can't make heads or tails of such huge commits. It's usually not a good idea to mix real changes, real fixes, with reformatting, because the diffs become really hard to inspect. If it's just reformatting you put "only reformatting" on the svn comment and people know what they're looking at. If you put "reformatting + this bug fixed + this refactoring..."... well, it's harder.

Soo... I doubt I can find the glitch no matter how much I try. I've taken shallow looks at the subject and I have no clue, and I suspect I'll remain clueless.
chuck_starchaser wrote:
And since you started doing other work in the branch than formatting, I believe the other work should be "backported" into trunk first - then the formatter do its thing. And fixing the macros is rather quick ;)
What?!!!
Are you saying I should remove the reformatting, leaving only the other changes? How would I do that?
You know what you changed - if you make a list of what macros you fixed, you just list them all and fixing them again is plain simple and fast - the hardest part was finding them.
Claudio... !!! :( :( :( you don't understand! I have fixed GAZILLIONS of problems. I've been working like 16 hours a day for I lost count how many days. It would take me a week to come up with a list of changes.
Just how many changes do you THINK I've made? 20? 50? No; it's like several hundred. I CAN'T make a list of them. Sorry.
And reformatting is NOT mixed with the other changes. Third time I'm saying this. Today yes, because I touched some files in the collide library, so might as well reformat them; but it's actually pretty clear in the diffs in TRAC
to differentiate formatting from real changes, because real changes are darker colors. Forget the email diffs;
I installed TRAC for a reason. Scroll down...
About unit_server.cpp... that one's harder. If you send a patch (svn diff of your working directory) I could inspect and review it - that's what I was talking about code review. It's nice, because it helps keep the source quality high, and because it's a nice place to exchange ideas and basically learn from each other and make coding decisions - the forum isn't made for that.
I'll see about installing a review board in wcjunction with your permission.
Looks to me like you just want to throw monkeywrenches, and trash all the work I've done for no reason.
Sure, like we have svn, but now the height of wisdom is to go around svn and produce patches and
whatnot?!! The changes I did to refactor GameUnit are contained in the change from r12632 to r12633. You
don't need a special diff from me; you can see them by scrolling down right here in r12633.
Just ignore the blank lines I added or deleted.
chuck_starchaser wrote:and what's the point?
Keeping things clean and not introducing any new bugs...
I've been testing not only flying around,
but with a bit of combat, buying and selling, and the upgrades and repairs. No problems. All I got is a
couple of graphical glitches, and you've decided you're not going to help me. Fine; I'll keep working
alone, then.
chuck_starchaser wrote:Does it matter, historically whether reformatting comes before or after macro fixing or unit refactoring?
...you've experienced it yourself. You HAD to fix macros because they broke reformatting. So reformatting MUST come AFTER fixing the macros, so that reformatting works right - or did I make a mistake somewhere in that reasoning?
If you fix the macros after the formatter made the mistakes, there's no telling what leftover mess could there be.
I disagree. The reformatting made the bad macros a lot easier to identify and edit. Secondly,
we wouldn't even know there were bad macros if I hadn't uncrustified.
In any case, fixing the problems with macros is as easy as telling uncrustify to force braces always. I don't want
to do that, because I don't want to hide the problem; I want to fix them.
chuck_starchaser wrote:And when you merge, if my understanding
is correct, the branch commits are not fused together; they show individually.
Ehm... not to my knowledge.
They do get fused together.

"svn merge" is a tool that incorporates the changes from a branch into your working directory, then you commit those changes - just one commit. If you want SVN history to show the commits separately (ie: they're different changesets, different features) you have to merge them step by step.
Well, that's good, then; I can then make two commits: One from the second reformatting revision, and a second commit with all the stuff since.
That's why I support task-oriented branches (one for reformatting and reformatting alone, one for separating unit.cpp into unit_server, etc...), so merging only merges changes of one task, not several.
I do apollogize for having got side-tracked with that refactoring, though I don't regret it one bit.
But in any case, the reformatting and the other changes are 95%+ separate commits.
chuck_starchaser wrote:The trial and error was without commits, so no dirt at all. Actually, the only dirt is I forgot to remove the #ifdef VEGASERVER_COMPILING's, but they affect nothing, since the makefile no longer defines it. I'll take them out before the next commit.
Glad you managed that - I usually inspect the patch just in case, and find one thing or another I had forgotten about. Just do "svn diff | less" and read through, if you want. A quick look is enough to remind you of things you did.
Thanks for the tip.
Anyways, to summarize:
  • r12625 is the reformatting revision. I haven't reformatted any files since then, except a dozen files in the collide folder I worked on today; --unless by "reformatting" you include things like adding blank lines between function definitions; I do that all the time; I can't stand seeing two functions touching each other; and uncrustify doesn't have a setting to separate them, believe it or not.
  • Everything since r12625 has been cleaning up, bug fixing, warning squashing and refactoring.
  • The game seems to be working well, except for the missing posters during loading, and a square billboard around local stars. I'm sure you and/or Safemode have SOME idea where in code the posters are loaded?
EDIT:
And why so much concern for bug-free-ness of a commit to trunk, anyways? It's not like this has to be 6.0 or nothing; is it?
It's just trunk; not some long-term support, ultra-stable release we're talking about. The game is playable as it is; so it
wouldn't be inconceivable to commit to trunk even without fixing the glitches. But I'm not asking for that; just asking for
a bit more respect for all the hard work and sleepless nights I've put in for the past week.
charlieg
Elite Mercenary
Elite Mercenary
Posts: 1329
Joined: Thu Mar 27, 2003 11:51 pm
Location: Manchester, UK
Contact:

Re: Code reformatting project

Post by charlieg »

I think klauss is looking for the ideal merge (i.e. merge fix A, merge fix B, then merge reformatting) in a situation where identifying fix A/B/etc would be an absolute nightmare.

I don't think he is meaning to disrespect the amazing effort you just put in. I also think that sometimes developers (like klauss) get stuck in the SCM mentality. The history can be useful but it isn't that important once the codebase moves on. I don't think there's that much to gain from splitting up chuck's changes - that's ideological thinking. Just merge as-is and note the things it fixes.
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Code reformatting project

Post by klauss »

chuck_starchaser wrote:I installed TRAC for a reason. Scroll down...
Cool... trac does make it look prettier and more readable.
But my point stands, the commit has a lot of files with changes unrelated to macros.
Ideally, I'd like a diff like this:

Code: Select all

@file whatever
- #define SWAP(a,b) tmp=a; a=b; b=tmp;
+ #define SWAP(a,b) do { tmp=a; a=b; b=tmp; } while(0)

@file whatever
- #define SWOOP(a,b) tmp=a; a=b; b=tmp;
+ #define SWOOP(a,b) do { tmp=a; a=b; b=tmp; } while(0)

...etc
But interspersed in that commit is reformatting stuff to beam_generic.cpp, for instance.

I get however what you mean, the files that do have fixes have fixes and only that. That's cool, you can help yourself in keeping track of those files using changelists:

An example from work:

Code: Select all

$svn cl manual_tests selection_test.py
$svn cl external_users sampling.py ManualDeliveryAPI.py
$svn status

--- Changelist 'manual_tests':
M      selection_test.py

--- Changelist 'external_users':
M      sampling.py
M      ManualDeliveryAPI.py
There SVN helps you keep track the related "changesets" in your working directory. You can diff only a set with "svn diff --cl manual_tests", commit only one set "svn commit --cl manual_tests", etc...

As long as you don't have two tasks involving the same file you'll be immensely happy with that ;)

And you'll help others by keeping the changes committed in a single commit all related.
chuck_starchaser wrote:
About unit_server.cpp... that one's harder. If you send a patch (svn diff of your working directory) I could inspect and review it - that's what I was talking about code review. It's nice, because it helps keep the source quality high, and because it's a nice place to exchange ideas and basically learn from each other and make coding decisions - the forum isn't made for that.
I'll see about installing a review board in wcjunction with your permission.
Looks to me like you just want to throw monkeywrenches, and trash all the work I've done for no reason.
Sure, like we have svn, but now the height of wisdom is to go around svn and produce patches and
whatnot?!! The changes I did to refactor GameUnit are contained in the change from r12632 to r12633. You
don't need a special diff from me; you can see them by scrolling down right here in r12633.
Just ignore the blank lines I added or deleted.
Ok, thanks for the revisions. Remember that only to you those revisions are obvious, for someone not closely involved in the process (and I'm not, your are, I'm only following it on the forum and cvs notifications), finding one specific change in SVN isn't as easy.

So you can make a patch doing: svn diff -r12631:12633 > unit_refactoring.patch. That's not circumventing SVN, that's the standard workflow for code review. You may like code review, read here
chuck_starchaser wrote:I've been testing not only flying around,
but with a bit of combat, buying and selling, and the upgrades and repairs. No problems. All I got is a
couple of graphical glitches
<snip>
The game seems to be working well, except for the missing posters during loading, and a square billboard around local stars. I'm sure you and/or Safemode have SOME idea where in code the posters are loaded?
From your description of the problem, it's not precisely a problem with loading as much as with OpenGL state tracking. I'll have to confirm it by actually seeing the glitches, but right now I don't think I have enough spare bytes on my /home partition for yet another build (I have trunk and the audio branch already active, with several hundred MB in build temporaries each). I'll have to make room, and you're working 16 hours a day on this, I only get less hours a weekend... don't bite my ass for not having time to catch up.

chuck_starchaser wrote:
...you've experienced it yourself. You HAD to fix macros because they broke reformatting. So reformatting MUST come AFTER fixing the macros, so that reformatting works right - or did I make a mistake somewhere in that reasoning?
If you fix the macros after the formatter made the mistakes, there's no telling what leftover mess could there be.
I disagree. The reformatting made the bad macros a lot easier to identify and edit. Secondly,
we wouldn't even know there were bad macros if I hadn't uncrustified.
In any case, fixing the problems with macros is as easy as telling uncrustify to force braces always. I don't want
to do that, because I don't want to hide the problem; I want to fix them.
I think finding the macros was a good thing, and fixing them a must.
Still, I don't trust uncrustify to be as benign as you seem to think... I'd fix the macros on the crustified version and then re-uncrustify, just to be safe. Who knows how confused uncrustify became when it found macro calls without the colons:

Code: Select all

DO_SOMETHING_SETUP() // <-- missing colon
code code and more code
...and how many other instances of uncrustify-confusing code like that
chuck_starchaser wrote:Well, that's good, then; I can then make two commits: One from the second reformatting revision, and a second commit with all the stuff since.
That would be great :D
chuck_starchaser wrote:But in any case, the reformatting and the other changes are 95%+ separate commits.
Separate commits aren't enough when you're done and get to the task of actually merging - you'll notice it's hard to separate the changesets post-facto. From my experience, the most comfy and quick way of doing this is to open a branch for each major task you undertake, and organize your minor tasks with changelists.
In fact, kernel devs also think like that, because they invented (and adopted) git, which does just that.
I imagine I'd be in heaven with git ;)
Too sad nobody uses it at work :(
chuck_starchaser wrote: And why so much concern for bug-free-ness of a commit to trunk, anyways? It's not like this has to be 6.0 or nothing; is it?
Because I fear the bugs introduced by uncrustify will be the hardest to track. If they're not caught right now, they'll be a pain in our collective asses.
chuck_starchaser wrote:It's just trunk; not some long-term support, ultra-stable release we're talking about.
Trunk's all we've got... we don't have stable branches.
chuck_starchaser wrote:The game is playable as it is; so it
wouldn't be inconceivable to commit to trunk even without fixing the glitches.
Again, normal bugs I wouldn't worry... batch processing-related bugs are another story. They tend to be subtle bugs that stem from a missing colon, brace or whatever, and mighty hard to spot. Furthermore, commit history doesn't help you narrow down the search space, because batch processing-kind commits change every line of every file.
chuck_starchaser wrote:But I'm not asking for that; just asking for
a bit more respect for all the hard work and sleepless nights I've put in for the past week.
Sorry, I realize even suggesting to redo or even review something that took so much effort must sound annoying at best, insulting at worst.

But look at it like this... if you manage to refactor unit to not use templates correctly, without introducing new bugs, and in such short amount of time, you'll have beaten many many devs that tried ;)
Including me.

Take your time to do it right, then. Nobody does anything right in VS, I figured you for the type that would quickly embrace "the right way(tm)". It will be a snowball. Today I pester you to not be sloppy, tomorrow you pester me about a sloppy commit (I tend to be sloppy with VS because of lack of time). First thing you know, everybody's submitting review requests and the code is improving. It's getting documented... :,-) snif... tears of joy...

@charlieg: well, yes, it's a bit ideological. But it's a bit practical just as well: chuck's work took him by surprise by introducing at least one bug nobody knows where it comes from yet, one bug introduced by an automatic source formatter. As I said... I hope that organizing for a perfect merge will help make the problem evident. Because that's what organizing does... putting things in perspective for easier review.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

The moral of this story, always write down what you are doing so you can backtrack.

Good work chuck, just try not to do so much at once w/o keeping track of WHAT you did. I know its easy to go from one project into several but breaking something and then going about cleaning and debugging it.
klauss wrote:Trunk's all we've got... we don't have stable branches.
Actually our stable branch (.5) is sitting under tags.. though what we really need is a code fork. Maybe create a new vs fork so we can have more control over the project either at sf or a github (or use sf for svn and a git repo site for git). Crazy thought...

Who can start a branch for me for win32?? I still dont have svn access and need to get my work up eventually. My only two choices are ask for svn access to chucks server or add a vs branch to my sf project. 2nd might be easier.. maybe I should do that.
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Code reformatting project

Post by chuck_starchaser »

klauss wrote:
chuck_starchaser wrote:I installed TRAC for a reason. Scroll down...
Cool... trac does make it look prettier and more readable.
But my point stands, the commit has a lot of files with changes unrelated to macros.
Ideally, I'd like a diff like this:

Code: Select all

@file whatever
- #define SWAP(a,b) tmp=a; a=b; b=tmp;
+ #define SWAP(a,b) do { tmp=a; a=b; b=tmp; } while(0)

@file whatever
- #define SWOOP(a,b) tmp=a; a=b; b=tmp;
+ #define SWOOP(a,b) do { tmp=a; a=b; b=tmp; } while(0)...etc
That's ideal for a diff; NOT ideal for Humans, and workflow.
You're taking the exact same position HellcatV took, namely that there should only be changes in sources that cause
the executable to change; or perhaps that cosmetic changes should be completely separate. Well, that's just not
realistic.
When I'm working with code I notice things that could make the code more readable, a variable name that
would be better, a const that was missed, etceteras. I cannot completely switch myself off all concerns except a
totally focused solution to one problem. Improving the code is something that you do as you work with it.
Uncrustify doesn't add a line between functions, so I do it whenever I see two functions stuck together. What's the
big deal? I just showed you that trac makes it clearer. Well, Meld makes it even clearer. We've got visual tools; let's
use them. We shouldn't restrict our actions to complete paralysis, just so that old style diffs look clean. What needs
to look clean is the code; NOT the diffs. The integral; NOT the derivative.
But interspersed in that commit is reformatting stuff to beam_generic.cpp, for instance.
Yes, and I will continue to do so.
Sorry.
There's just no better time to refactor a class than when you're working with it --for no matter how unrelated a reason.
And to expect any less is to adopt HellcatV's stance and forestall continuous improvement.
I get however what you mean, the files that do have fixes have fixes and only that. That's cool, you can help yourself in keeping track of those files using changelists:
Not exactly; like I said, when I'm working with code I tend to see a lot of things and try to
fix them and improve them. It's like my changing the "image" member of Unit to "pImage". That was not necessary to refactoring; it's
just a more appropriate name for a pointer. And inside *pImage there's two pointers I changed into auto_ptr<>'s, and later back to
pointers; and that wasn't necessary to the refactoring. Just because one works with code doesn't mean one should stop being human.
I care about aesthetics, and I'm not going to repress myself from improving the organization of a class I'm working with just because
it's not specifically part of the work I'm doing. It's you that should try to use more visual tools, that make formatting changes distinct
visually from real code changes, such as Trac and Meld. To hell with diff.
An example from work:

Code: Select all

$svn cl manual_tests selection_test.py
$svn cl external_users sampling.py ManualDeliveryAPI.py
$svn status

--- Changelist 'manual_tests':
M      selection_test.py

--- Changelist 'external_users':
M      sampling.py
M      ManualDeliveryAPI.py
That's totally Greek to me. No idea what that is, other than it involves svn status.
There SVN helps you keep track the related "changesets" in your working directory. You can diff only a set with "svn diff --cl manual_tests", commit only one set "svn commit --cl manual_tests", etc...
Greek to me, again.
I've been struggling with svn for hours; my own problem. I only have the vegastrike folder of the branch I created,
but svn doesn't allow me to merge from trunk/vegastrike to branches/reformat/vegastrike. It says that
"vegastrike" is not under version control. Found forum posts from people who had the same problem. One
of them hacked the .svn folders to do it. I got around all that by checking out a fresh copy of trunk/vegastrike,
and with local folders I CAN merge from one to another. So, finally I got a merge started (just to integrate your
premul alpha commit), but now I got this diff I don't understand, opened in vim, a program I don't know well at
all. And I've slept maybe 4 hours in the past 3 days, and eaten twice; so I'm going nuts. But anyhow, what were
we talking about? Yeah, svn diffs and all that. I'm going to sleep on it :)
As long as you don't have two tasks involving the same file you'll be immensely happy with that ;)
To Hell with that.
And you'll help others by keeping the changes committed in a single commit all related.
That's not realistic. I don't have that much control of my sphincter, let alone my creative juices. Let the tools serve us; not us serve the tools.
chuck_starchaser wrote:
About unit_server.cpp... that one's harder. If you send a patch (svn diff of your working directory) I could inspect and review it - that's what I was talking about code review. It's nice, because it helps keep the source quality high, and because it's a nice place to exchange ideas and basically learn from each other and make coding decisions - the forum isn't made for that.
I'll see about installing a review board in wcjunction with your permission.
Looks to me like you just want to throw monkeywrenches, and trash all the work I've done for no reason.
Sure, like we have svn, but now the height of wisdom is to go around svn and produce patches and
whatnot?!! The changes I did to refactor GameUnit are contained in the change from r12632 to r12633. You
don't need a special diff from me; you can see them by scrolling down right here in r12633.
Just ignore the blank lines I added or deleted.
Ok, thanks for the revisions. Remember that only to you those revisions are obvious, for someone not closely involved in the process (and I'm not, your are, I'm only following it on the forum and cvs notifications), finding one specific change in SVN isn't as easy.
I describe the contents of my commits with the message that goes with it. Not sure if others do; but I do.
So you can make a patch doing: svn diff -r12631:12633 > unit_refactoring.patch. That's not circumventing SVN, that's the standard workflow for code review. You may like code review, read here
I'm a big fan of team work and peer review; I'm just not getting any.
chuck_starchaser wrote:I've been testing not only flying around,
but with a bit of combat, buying and selling, and the upgrades and repairs. No problems. All I got is a
couple of graphical glitches
<snip>
The game seems to be working well, except for the missing posters during loading, and a square billboard around local stars. I'm sure you and/or Safemode have SOME idea where in code the posters are loaded?
From your description of the problem, it's not precisely a problem with loading as much as with OpenGL state tracking. I'll have to confirm it by actually seeing the glitches, but right now I don't think I have enough spare bytes on my /home partition for yet another build (I have trunk and the audio branch already active, with several hundred MB in build temporaries each). I'll have to make room, and you're working 16 hours a day on this, I only get less hours a weekend... don't bite my ass for not having time to catch up.
I only bite ass when my ass is bitten first. I don't think it has anything to do with OpenGL; tell you why: because the game loads faster than ever; so it's just not doing anything related to loading those images, IMO.

chuck_starchaser wrote:
...you've experienced it yourself. You HAD to fix macros because they broke reformatting. So reformatting MUST come AFTER fixing the macros, so that reformatting works right - or did I make a mistake somewhere in that reasoning?
If you fix the macros after the formatter made the mistakes, there's no telling what leftover mess could there be.
I disagree. The reformatting made the bad macros a lot easier to identify and edit. Secondly,
we wouldn't even know there were bad macros if I hadn't uncrustified.
In any case, fixing the problems with macros is as easy as telling uncrustify to force braces always. I don't want
to do that, because I don't want to hide the problem; I want to fix them.
I think finding the macros was a good thing, and fixing them a must.
Still, I don't trust uncrustify to be as benign as you seem to think...
Well, you predicted a week ago or two that using a code formatter was going to be hell on earth, and uncrustify proved you wrong. Naturally you don't trust it. Thing is, I uncrustified the entire goddam sources and they was still building and running. What more proof do you need?
I'd fix the macros on the crustified version and then re-uncrustify, just to be safe. Who knows how confused uncrustify became when it found macro calls without the colons:

Code: Select all

DO_SOMETHING_SETUP() // <-- missing colon
code code and more code
...and how many other instances of uncrustify-confusing code like that
I can't say uncrustify likes to see that any more than I do, but it's seen worse and not been confused. What really confused the heck out of uncrustify was the python wrapper macros that open parenthesis and braces but don't close them, and viceversa. There was one file full of those, can't remember which now, that I had to leave crusty.
In any case, what uncrustify does, if it can't be sure of doing a decent job, is complain and quit, rather than mess up the code.
chuck_starchaser wrote:Well, that's good, then; I can then make two commits: One from the second reformatting revision, and a second commit with all the stuff since.
That would be great :D
Consider it done, if I figure how.
chuck_starchaser wrote:But in any case, the reformatting and the other changes are 95%+ separate commits.
Separate commits aren't enough when you're done and get to the task of actually merging - you'll notice it's hard to separate the changesets post-facto. From my experience, the most comfy and quick way of doing this is to open a branch for each major task you undertake, and organize your minor tasks with changelists.
Haven't merged yet, but I have a merge stalled there, and vim opened, and i see that the right tools have yet to be born, perhaps; these are certainly NOT it. The process of merging should be a lot more visual, intuitive and smooth. VIM is hell to even remember the most basic commands. The merge tool should show the two source files
left and right, and the ongoing merge in the middle; you point and drag blocks from either side; have a slider at the top for setting the
preferred balance in formatting closeness to either source; and the tool should understand the language you're dealing with, and show
the code as blue when the sources match, green for a source changed that agrees with the merge so far, yellow if of dubious comptibility,
like warning level, and red if it would not compile.
In fact, kernel devs also think like that, because they invented (and adopted) git, which does just that.
I imagine I'd be in heaven with git ;)
Too sad nobody uses it at work :(
I could install it in the server if there's consensus.
chuck_starchaser wrote: And why so much concern for bug-free-ness of a commit to trunk, anyways? It's not like this has to be 6.0 or nothing; is it?
Because I fear the bugs introduced by uncrustify will be the hardest to track. If they're not caught right now, they'll be a pain in our collective asses.
Uncrustify didn't introduce bugs; it uncovered them. There's a HUGE difference.
chuck_starchaser wrote:It's just trunk; not some long-term support, ultra-stable release we're talking about.
Trunk's all we've got... we don't have stable branches.
We can set a tag; call it "Stable; --pre-reformatting".
chuck_starchaser wrote:The game is playable as it is; so it
wouldn't be inconceivable to commit to trunk even without fixing the glitches.
Again, normal bugs I wouldn't worry... batch processing-related bugs are another story. They tend to be subtle bugs that stem from a missing colon, brace or whatever, and mighty hard to spot. Furthermore, commit history doesn't help you narrow down the search space, because batch processing-kind commits change every line of every file.
Ditto. The chances that uncrustify CAUSED a bug are pretty much nil. They'd have to be pretty sophisticated "bugs" for them to get past the compiler and only affect run-time behavior. A programming language is a fire-wall against precisely that. Most mistakes are caught by the compiler. What causes the code to be so fragile in the first place, is things like publically exposed data members of classes, old style casts, and macros. You've got uncrustify to thank for exposing these weaknesses, so we can fix them.
chuck_starchaser wrote:But I'm not asking for that; just asking for
a bit more respect for all the hard work and sleepless nights I've put in for the past week.
Sorry, I realize even suggesting to redo or even review something that took so much effort must sound annoying at best, insulting at worst.
What I found most insulting was actually your asking me for list of changes, as if I had been so inefficient with my time that such lists would be short and sweet. I made gazillions of changes.
But look at it like this... if you manage to refactor unit to not use templates correctly,
You mean to use templates correctly?
without introducing new bugs, and in such short amount of time, you'll have beaten many many devs that tried ;)
Including me.
I told you, Klauss; remember? Couple of months ago. If it can't be done, give it to me. That's my middle name.
Take your time to do it right, then.
Are you suggesting I didn't?
Nobody does anything right in VS, I figured you for the type that would quickly embrace "the right way(tm)". It will be a snowball. Today I pester you to not be sloppy, tomorrow you pester me about a sloppy commit (I tend to be sloppy with VS because of lack of time). First thing you know, everybody's submitting review requests and the code is improving. It's getting documented... :,-) snif... tears of joy...
Amen.
Post Reply