klauss wrote:
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
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.