Code reformatting project [DONE]

Development directions, tasks, and features being actively implemented or pursued by the development team.
Post Reply
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:That's ideal for a diff; NOT ideal for Humans, and workflow.
Diff couldn't care less. I care. I have to read less for that.
But if you don't want my help just go on committing like that. I can't find a change among a thousand unrelated lines, trac or no trac.
chuck_starchaser wrote: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.
Minor cosmetic changes. Using that argument to justify using uncrustify on a file is like saying "fuck you". At the very least, uncrusfity, commit the uncrustification, and then go on working.
chuck_starchaser wrote: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.
Visual tools can't separate the relevant changes from the irrelevant.
chuck_starchaser wrote:
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.
And to answer like that is to say "fuck you".
chuck_starchaser wrote: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;
That's arguable. With that same argument I could have named it piImage (pointer to image). Then apuiImage (auto-ptr to Unit Image). Then everything gets out of control. Hungarian notation is widely hated for getting out of control rather easily.
chuck_starchaser wrote: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.
Ehm... merging doesn't produce diffs, but anyway, check out one of the graphical diff viewing tools out there (kdiff, for instance).
chuck_starchaser wrote:
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.
I give up.
chuck_starchaser wrote:I'm a big fan of team work and peer review; I'm just not getting any.
I told you why. 15000 lines changed, 50 are const changes, 50 are macros, the rest reformatting. I won't spend my day finding the 50 lines I care about in an ocean of 15000, as much as you won't spend the effort to commit them separately.
chuck_starchaser wrote: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.
Good tip.

chuck_starchaser wrote: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?
Yeah... it didn't make the game not load images or anything like that :roll:
chuck_starchaser wrote: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.
You must have a smarter compiler than mine.
chuck_starchaser wrote: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.
Don't worry. Won't happen again.
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 »

Well, I give up too.
I've gone through all the source folders twice, and can't find any more macros to fix; so I guess
it's something else. And I'm totally exhausted.
Deus Siddis
Elite
Elite
Posts: 1363
Joined: Sat Aug 04, 2007 3:42 pm

Re: Code reformatting project

Post by Deus Siddis »

So is this good project dead without usable results, just resting or has it fixed many problems with the difficult code base but can't go any further?
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

Probably in a rest phase.
Because of YOU Arbiter, MY kids? can't get enough gas. OR NIPPLE! How does that mkae you feeeel? ~ Halo
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 highly doubt the project is dead. It just needs somebody with a slightly better understanding of the graphics pipeline (probably klauss) to debug it a bit or at least suggest where to look.
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Code reformatting project

Post by safemode »

I've been spending much of my "free" time at work combing through 12627 (the first commit that mentions the glitches) looking for any inconsistencies at all.

This is the first one i've found. It appears to have pulled the function call out of a block. I'm almost 100% sure this is not intentional, though it seems to not be connected to the glitches described.

Line 110-112:
http://vegastrike.svn.sourceforge.net/v ... hrev=12627


There are also, randomly, other formatting related issues i've come across too. But they shouldn't have any impact on the compiled code.


Still looking. One thing that would help is if we can verify (i'm not near a vs compile) that startup backgrounds and stars are the only things effected. The commit log is all i have to go by.
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 »

safemode wrote:This is the first one i've found. It appears to have pulled the function call out of a block. I'm almost 100% sure this is not intentional, though it seems to not be connected to the glitches described.

Line 110-112:
http://vegastrike.svn.sourceforge.net/v ... hrev=12627
That's most likely it. Good catch.

It's in the right place to mess with GL state in a way that's consistent with chuck's description of the bug (I still couldn't build the branch at home, though I'll probably succeed today when I get back).
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 »

yea, i just looked at the code for displaying the splash screen. it creates an Animation ....so it looks like it is it.


edit: hopefully if this fixes the glitches. we can get back on track with merging much of these changes back to trunk.


I have one suggestion before merging back to trunk (and only merge vegastrike ...heh, didn't need to branch the entire source tree :) it will make things quicker on the svn serverside)

Since everything was kinda rushed together and it would take too much time than those have effort to spend to separate out the reformatting changes from the macro fixes and other fixes related to reformatting that were done at the same time .. You might be able to minimize the noise over your non-uncrustify patches by doing the following:

Run uncrustify using your final settings on a new pull of trunk. svn diff this to your branch and save that file as "reformat-branch.diff" Commit the uncrustify changes in the trunk. Then apply your reformat-branch.diff to the trunk pull. Commit these changes as "merging reformat branch". This should minimize the amount of uncrustify reformatting infecting the other changes you made (all the manual stuff). This will effectively merge your branch to trunk, you can check by using svn merge between the two again and see if it finds any diffs. It shouldn't.

It wont be perfect, but it is better than a straight merge.
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 »

Nice read with links to more nice reads

I can't find a solution to this particular problem though in there, but at least it will help you chuck when you have to actually merge stuff. Which can be tricky.
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 »

just remember to always --dry-run a merge first to make sure it will do what you expect. But my previous suggestion would have you not directly merge the branch to trunk, rather in order to separate out as much as possible the uncrustify changes from the manual ones, to uncrustify the trunk and commit that, then create a diff against the new trunk and your branch and commit that. This second commit should contain all of the manual changes and a minimal amount of uncrustify changes.

in the end, your branch and trunk should be identical.
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 »

MIGHTY great catch, Safemode; that's gotta be it.
So, uncrustify DOES have a bug, after all. I had observed its taking comment lines from outside blocks and moving them into blocks; but never noticed it's doing it with code lines.
klauss wrote:Nice read with links to more nice reads

I can't find a solution to this particular problem though in there, but at least it will help you chuck when you have to actually merge stuff. Which can be tricky.
I'll read that in a minute. Just wanted to say, this is my first experience with branching and all that; and there's a lot of things I won't do again. To my credit, though, when I merged your premul alpha commit, I did it in vim. :D
safemode wrote:I have one suggestion before merging back to trunk (and only merge vegastrike ...heh, didn't need to branch the entire source tree :) it will make things quicker on the svn serverside)

Since everything was kinda rushed together and it would take too much time than those have effort to spend to separate out the reformatting changes from the macro fixes and other fixes related to reformatting that were done at the same time .. You might be able to minimize the noise over your non-uncrustify patches by doing the following:

Run uncrustify using your final settings on a new pull of trunk. svn diff this to your branch and save that file as "reformat-branch.diff" Commit the uncrustify changes in the trunk. Then apply your reformat-branch.diff to the trunk pull. Commit these changes as "merging reformat branch". This should minimize the amount of uncrustify reformatting infecting the other changes you made (all the manual stuff). This will effectively merge your branch to trunk, you can check by using svn merge between the two again and see if it finds any diffs. It shouldn't.

It wont be perfect, but it is better than a straight merge.
Thing is, if I run uncrustify on a new pull from trunk, it will look exactly like my first commit to the branch.
This is not to say there weren't formatting changes AFTER that first commit; but they were mostly manual formatting changes, like adding or removing blank lines, etceteras; or sometimes fixing things that uncrustify didn't do too well.
That's why, what I suggested to Klauss was that I commit the first revision to trunk as 1 commit; and all the rest as a second commit.
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 »

Indeed that was it. It works now. r12640
http://vegastrike.wcjunction.com/trac/timeline

By the way, the migration of comment lines into blocks that I mentioned I saw happening, also
happened across closing braces in cases in switch statements.
I think I'm going to grep -R case ./* and visually check them all.

EDIT:
Added a ticket.
http://vegastrike.wcjunction.com/trac/t ... angeset=on

EDIT2:
One thing that might be good would be to invite players to test the reformat branch before merging it.
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

oh great, a new head :lol: Glad you found a fix chuck, too bad you cant fix windows... I soooo hope win7 is better. If windows follows its release pattern it should be on par with xp. 95 sucked 98 was good.. me sucked, xp was good... vista didnt suck as much, win7??? Every other windows release seems to be good. Hope 7 didnt break the trend.
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 »

A coworker bought a computer with windows 7 pre-installed, but it came without a cd or dvd.
So, it seems fdisk should now work better than it ever did, at fixing windows... :D

In star_system_xml.cpp, line 853, there's a second set of braces for a case statement, where
the first set of braces has a break; so, it amounts to dead code.

EDIT:
Correction: star_system_xml.cpp is a mess!
Last edited by chuck_starchaser on Tue Feb 23, 2010 4:50 am, edited 1 time in total.
pheonixstorm
Elite
Elite
Posts: 1567
Joined: Tue Jan 26, 2010 2:03 am

Re: Code reformatting project

Post by pheonixstorm »

chuck_starchaser wrote:A coworker bought a computer with windows 7 pre-installed, but it came without a cd or dvd.
Sounds like he bought a HP computer, they tend to have a seperate partition for reinstalling the OS. Not sure who else is like that.. Dell sends out install disks, but you can only installed an activated copy of a dell system (and my Dell is only an antiquated GX 150.. PIII 800)
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 »

star_system_xml.cpp, line 1231, switch statement with a single case (and no default,
of course; that's the default).
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:star_system_xml.cpp, line 1231, switch statement with a single case (and no default,
of course; that's the default).
I don't think it's that bad, see, it's a case on the attribute being processed. It's just that there's only one applicable attribute, and keeping the form of all attribute lookups makes it clearer. And easier to edit later (ie: no need to change the if into a switch).
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 »

Whatever is decided, it would be good to go through any important files like unit* files and any personal favorites and fix any uncrustify glitches. In the revision i was looking at, i saw many instances of inconsistencies in the formatting.

One thing i noticed was that generally, we went from single line with braces to single line without braces. Some files this is reversed.

We also went from putting multiple things on one line to splitting them. Also, we split single line if(something)block ....sometimes this is not put on separate lines..

Some function calls are unnecessarily split into separate lines per argument. This is good with long arguments, but many do not need it (multiple lines with a single digit != good).

all that manually touchup stuff. :) Overall though, vast improvement.
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:Whatever is decided, it would be good to go through any important files like unit* files and any personal favorites and fix any uncrustify glitches.
I've been grepping for case one folder at a time, looking for possible uncrustify bugs with braced cases. So far I've checked all cases in the main, src folder, and I'm half-way through the cmd folder and sub-folders. Haven't found a bug yet, but...
In the revision i was looking at, i saw many instances of inconsistencies in the formatting.
...indeed... and I've been correcting such inconsistencies as I go along. Not all of them; just the ones that hit my eye hard.
((Note: If I do find any "bugs", I will just note them down, so that my next commit is purely cosmetic; then I will fix such bugs, if any, for a further commit, without cosmetics.))
One thing i noticed was that generally, we went from single line with braces to single line without braces. Some files this is reversed.
What I did is I told uncrustify to remove braces around single statement blocks, BUT NOT if they are part of a chain of if, [else if]..., else chains where any of the other blocks have braces, --as chains containing some braced and some unbraced blocks are harder to read. So, in some cases uncrustify may remove braces around a single statement block, but in other cases it may force them.
We also went from putting multiple things on one line to splitting them.
I did this on purpose; sometimes I find it much harder to find something if it's in the same line, to the right of something else. I hope this doesn't disagree too much with other people's preferences.
Also, we split single line if(something)block ....sometimes this is not put on separate lines..
This appears to be an unfortunate bug in uncrustify. It was supposed to put them on separate lines, but sometimes it doesn't. I've been manually correcting such cases whenever I see them.
Some function calls are unnecessarily split into separate lines per argument. This is good with long arguments, but many do not need it (multiple lines with a single digit != good).
I totally agree, but there's just no way to inject more intelligence into uncrustify in this issue. It decides whether to split arguments based on the maximum width parameter you give it, so if it doesn't do this kind of weird thing with one function, it does it with another, depending on width. It is NOT set to put one argument per line, by the way; it's just what happens when the argument list begins too close to the maximum width.
I think, though, it is preferable to the alternative, which is to have uncrustify ignore formatting of function declarations and calls, --leave them alone. What I've seen in many places in the code is horrendously long lines where one or more parameters passed to a function are function calls themselves with multiple parameters. At least, the way uncrustify rearranges them, the parameters of the inner function calls are aligned at different indentations from those of the main function call, so you can easily tell them apart. The vertical space taken up is unsightly but less confusing.
all that manually touchup stuff. :) Overall though, vast improvement.
Glad you like it; I think it's orders of magnitude easier to work with, now.
Even I understand some of it... :)
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: ((Note: If I do find any "bugs", I will just note them down, so that my next commit is purely cosmetic; then I will fix such bugs, if any, for a further commit, without cosmetics.))
:D :D :D :D :D :D :D :D :D :D :D :D :D :D :D :D :D :D :D
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 »

Power of two happiness! :)

Klauss, where we fundamentally disagree is about naming of pointers. I have never pushed for Hungarian notation,
and everybody would agree it's awful. But prefixing p for pointers is commonly used; and it makes things much clearer.
It is RIDICULOUS to have a variable named "image" and then to find out it's a pointer.
There's no need for using a different notation for smart pointers; they serve the same basic function, as far as the
syntax is concerned, so I'd use the same p prefix for them. And there's no need for r for references, as the syntax is
the same as for value types. Also, there's no need to p on iterators, as long as it's clear they are iterators; and their
syntax is not exactly that of pointers (( (*iter).func() rather than iter->func(), can't remember why )), anyways.
But distinguishing between data and something that points to data is as fundamental as fundamentals get.
By the way, this is the standard adopted in the book Design Patterns; --not that appeal to authority is my favorite
fallacy, but for those who just love it...

EDIT:
And yes, a pointer to a pointer should be a ppXXX.
EDIT2:
And no, the names of arrays are excepted, since they are meant to be used with subscripts; and we do, hopefully...
EDIT3:
And yes, functions that return a pointer should have a p (after get, if there's get, and before the type or thing;
--e.g.

Code: Select all

template <class T>
T * singleton<T>::pInstance()
{
    if( !pInstance_.get() )
        pInstance_.reset( new T() );
    return pInstance_;
}
.....................................
pMyBar = singleton<Bar>::pInstance();
And by the way, I think the trailing underscore for private class members is another good standard from Design Patterns.
Last edited by chuck_starchaser on Tue Feb 23, 2010 10:17 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 »

Oh, but auto_ptr is really different from shared_ptr (the first is noncopyable, which is a HUGE thing), which is different from basic pointer (which isn't smart and you have to free it).

And a value is different from a reference (a change to a value is local, to a reference not so)... etc...

There are sooo many things that are supposedly fundamental and meritory of their own prefix, that you in fact end up re-inventing hungarian notation.

I used to do that, prefixing stuff. I found it didn't help, it just made things harder to read. It doesn't help because you have to know the types of things very closely already to even attempt to write correct code. So it's far more important to have declarative (clear) typedefs than it is to prefix variables by their type.

Code: Select all

std::list< std::pair< std::map<std::string, int>*, std::string > > &list;
Hm?
Now...

Code: Select all

NamedMappingList &list;
Ah...

So...

Code: Select all

std::shared_ptr<Unit> pUnit;
or

Code: Select all

UnitPtr unit;
?

You can convey a lot more information in a properly named typedef than in variable prefixes. And in C++, variable declarations are usually close to their use.
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:... And in C++, variable declarations are usually close to their use.
This is the best argument; and in fact my big cow with image being a pointer is because it's also a public data member of Unit, and is accessed from all over the code; declared as close to its use as the furthest galaxy... But I agree, with proper encapsulation it's not an issue.
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 »

Having said that, a good argument for prefixing pointers with p is that there shouldn't be any.
What do we need pointers for?
Pimpl's have a pointer, but that's their private business.
For iterating we've got iterators.
For subscripting we have square brackets.
For most other things we've got references.
Post Reply