I was bored, so I started looking through the q3asm code and realized how much of the code in the cmdlib files wasn't actually being used, so I went through and reduced things to one single pair of files at this point. The code got cleaned up in a couple of places by this, but mostly it removed a lot of unused/unreachable code and dead-end codepaths. Since the diff's would be rather monstrous, I'll attach the new q3asm.c, q3asm.h, and Makefile directly since this work can remove all other files in the src/tools/asm directory except those three.
(In reply to comment #4)
> This probably won't apply anymore.
Unless there have been modifications to the trunk/src/tools/asm directory that aren't on the icculus.org SVN repository, this set of files still replaces the entire dirctory contents.
I'm inclined to not apply this patch; it's not really broken, so I don't want to make serious changes. If this was just trimming, that'd be good, but this patch is an awful lot of surgery.
--ryan.
(In reply to comment #6)
> I'm inclined to not apply this patch; it's not really broken, so I don't want
> to make serious changes. If this was just trimming, that'd be good, but this
> patch is an awful lot of surgery.
>
> --ryan.
Would you prefer I re-submit it as a series of incremental patches to remove the dead-end code paths and what-not in more bite-sized chunks?
Or is any code clean-up patch to the src/tools/asm directory going to simply be rejected no matter how I format them? I've waited patiently for 3 years, suddenly having 'WONTFIX' slammed in my face kinda stings with such a vague reason of "it's not really broken" when there's a crapton of stuff in that directory that has no business being there.
Would you prefer I re-submit it as a series of incremental patches to remove
the dead-end code paths and what-not in more bite-sized chunks?
Or is any code clean-up patch to the src/tools/asm directory going to simply be
rejected no matter how I format them? I've waited patiently for 3 years,
suddenly having 'WONTFIX' slammed in my face kinda stings with such a vague
reason of "it's not really broken" when there's a crapton of stuff in that
directory that has no business being there.
You haven't actually submitted any patches, so it's difficult to see what you're changing. It looks like you're collapsing all the headers down into fewer files? Is that all or are there functional changes too?
The first red flag was when I diffed q3asm.c against the new version and saw this...
- end = I_FloatTime ();
+ end = (double)time(NULL);
report ("%5.0f seconds elapsed\n", end-start);
...while that line isn't all that important in itself, it raised a few red flags:
- Replacing existing infrastructure with platform calls isn't an improvement, it's just a code reduction.
- Replacing existing code with incompatible code (time() returns seconds, not sub-seconds) seems sloppy at best.
Yes, I know I_FloatTime() currently has the sub-second code commented out, but that's not the point. There's removing dead code, and there's this. Scalpels and hatches.
Also, this file is out of date with several fixes that are in svn. This isn't your fault, since this file has been sitting in Bugzilla for ages, but it makes it more difficult to apply it now.
And finally, I don't see the value in crunching this down into one file anyhow.
If there's some dead code to remove, I'm not against that, but we don't have to be overly aggressive about it.
--ryan.
Created attachment 1106 [details] q3asm.hCreated attachment 1107 [details] q3asm.cCreated attachment 1108 [details] MakefileThe first red flag was when I diffed q3asm.c against the new version and saw this... - end = I_FloatTime (); + end = (double)time(NULL); report ("%5.0f seconds elapsed\n", end-start); ...while that line isn't all that important in itself, it raised a few red flags: - Replacing existing infrastructure with platform calls isn't an improvement, it's just a code reduction. - Replacing existing code with incompatible code (time() returns seconds, not sub-seconds) seems sloppy at best. Yes, I know I_FloatTime() currently has the sub-second code commented out, but that's not the point. There's removing dead code, and there's this. Scalpels and hatches. Also, this file is out of date with several fixes that are in svn. This isn't your fault, since this file has been sitting in Bugzilla for ages, but it makes it more difficult to apply it now. And finally, I don't see the value in crunching this down into one file anyhow. If there's some dead code to remove, I'm not against that, but we don't have to be overly aggressive about it. --ryan.