[karajorma]

yep. Whoever was coding this stuff was pretty tired, and these are bug causing problems that are almost impossible to track down

That sort of mistake is pretty easy to make. The reverse case is pretty easy too (and is even harder to spot if your programming language supports short-circuit evaluation as in that case it may actually be perfectly valid code even though it's not what you intended to do).

Must admit, that's why I always break statements like that down into bite-sized sections, it's so easy to sink into Boolean hell.

That sort of stuff is one of my biggest C programming pet peeves. The compiler should be screaming at the coder that what he's just done is nonsense.

There are also a fair few cases of this kinda thing (ambiguous ordering - what happens is compiler dependent):

Code: [Select]

if ( !Player->flags & PLAYER_FLAGS_MATCH_TARGET ) {


Yeah, that's almost certainly meant to have had everything after the NOT in brackets.

This one has got me baffled:

Code: [Select]

if ( (PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) {


Almost certainly was meant to read if ( (Popup_flags & PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) . I have no idea what changing this will do though. I can take a look.

This one will always return true if sv->type is not zero:

Code: [Select]

if(sv->type && SEXP_VARIABLE_NUMBER)


Definitely an error. Where exactly did you see it? I'll take a look to see if it needs any further fixing.

[portej05]

I'm bundling these all together, I'm just about to play through some missions to see if there are any obvious adverse affects. Some of these changes could cause really subtle errors.

EDIT: Most of these changes are not bug related. I'm going to play through a few missions to see if there are any obvious problems. I've attached a diff against 5204.

[karajorma]

I suspect a bunch of them have always caused bugs. For instance Wanderer reports that variable handling in lua has always been quirky. The if(sv->type && SEXP_VARIABLE_NUMBER) error (and the same one for strings below it) would definitely explain that. It simply wouldn't work for string variables as it would try to atoi() them into a number.

You can go ahead and fix that one too.

[karajorma]

For the PF_ALLOW_DEAD_KEYS one, That line was the only one in the code that refers to PF_ALLOW_DEAD_KEYS in the first place. I'm wondering if we should simply remove the reference to PF_ALLOW_DEAD_KEYS and only check (Game_mode & GM_IN_MISSION) ) or simply comment out the entire statement.

Not really certain to be honest.

[portej05]

See attached (numerous, small changes)

EDIT: Attachment eaten. Out of date.

[karajorma]

Looks good to me. Not so sure about your fix for Mantis 1780 though. Fixing the FRED issue by hobbling output in FS2 as well doesn't sit right with me somehow.

But I'll try to figure out what to do about that (if anything) and post it on the Mantis report instead of here.

[portej05]

I thought about that, but reasoned it out that if really long text message boxes are causing problems in FRED, really long text message boxes are probably causing problems in FS2.

[karajorma]

True.

Anyway I've just noticed that on the MSVC 2008 project files the warning level for debug is set to 3 not 4 for FRED and code. I think it would be a good idea to bump that up a bit.

[portej05]

Alrighty, everything we've chatted about today is in the attached diff.
I've started to add some of the SAL annotations to help with preFAST, if you see one that is wrong, please let me know.
I'm pretty sure I got the ifdefs right for SAL (check the bottom of pstypes.h after applying the diff), but if someone can compile with something that is VC++ that'd be great.
Played through a bunch of ST:R campaigns (AWESOME), I haven't noticed anything odd with the gameplay (other than a few NULL vectors) - however, there may be some subtle errors.
Feedback would be great.

EDIT: It's a diff against 5204 Trunk

[attachment deleted by ninja]

[karajorma]

I took a good long look at this and I'm somewhat confused as to what some of the changes actually do. For instance stuff like this.

Code: [Select]

-void send_autopilot_msg(char *msg, char *snd)
+void send_autopilot_msg( _In_z_ const char *msg, _In_opt_z_ const char *snd)

[portej05]

I just realised you're using VC6, so this will probably cause your compile to fail - will add a condition to check the VC version.

What it does is to provide the preFAST compiler with additional information on the intent of the function wrt to parameter use. In terms of a normal compile, it will do nothing at all - I'll also add a debug switch. By doing this, the noise from the preFAST step can be significantly reduced, and calling errors (both to library and internal code) can be picked up.
I'll try and dig up some good resources on it, but a good start is Michael Howards SAL/preFAST blog entry.

[karajorma]

I'm just not certain we should add something like that to SVN. I'm actually using 2008 but there are plenty who use VC6 and the Mac and Linux coders are going to have issues with it too.

I'll see about adding the rest of your fixes as soon as I can though.

EDIT : I was unsure of the reasons behind this change though

Code: [Select]

- output += wsprintf(output, "%08x: ", pStack);
+ output += wsprintf(output, "%08p: ", pStack);
in exceptionhandler.cpp. I'm a little confused as to why you're replacing the output in this way.


      
         

[portej05]

As far as I can tell, the %x specifier is unable to tell the size of the passed variable, and will always use the type 'int' for whatever is passed. So you'll never see an 8byte pointer (for a 64bit compile). The %p specifier will default to the pointer size - 4 bytes on a 32 bit compile and 8 on a 64bit compile - you could probably remove the %08.
pStack in that case is a pointer of type DWORD*, so it made sense to make that change (and it reduced the warning count)

[karajorma]

Well I've just committed all the other fixes. I'll remember to test and add this one in a bit.

[portej05]

I'm just not certain we should add something like that to SVN. I'm actually using 2008 but there are plenty who use VC6 and the Mac and Linux coders are going to have issues with it too.

They should all be #define'd to nothing under any non-preFAST platform (I need to change the VC version check though, and need to ensure that that includes those builds that aren't /analyze under VC++), so it shouldn't cause a problem, but can understand the hesitation.

If nothing else, it'll mean a few people get to scour the entire code base, and potentially add some in-code documentation for each function (useful!) and hopefully flag dodgy stuff they see along the way.

[karajorma]

Well I'd like to hear the comments of the other coders but personally I feel that although it reduces the number of false positives it also makes the code much harder to read. Especially for anyone looking at the code for the first time.

[chief1983]

If it's only used in problematic areas (sounds like that was the intent?) and commented where used it probably wouldn't hurt readability enough to get in the way of its potential benefits.  I'm not sure I understand it fully yet though.

[portej05]

Well I'd like to hear the comments of the other coders but personally I feel that although it reduces the number of false positives it also makes the code much harder to read. Especially for anyone looking at the code for the first time.

The code is a bit intimidating for anyone for the first time

[portej05]

Here's one not caught by preFAST (was actually looking for something else)

3ddraw.cpp:

Code: [Select]

//like g3_draw_poly(), but checks to see if facing.  If surface normal is
//NULL, this routine must compute it, which will be slow.  It is better to
//pre-compute the normal, and pass it to this function.  When the normal
//is passed, this function works like g3_check_normal_facing() plus
//g3_draw_poly().
//returns -1 if not facing, 1 if off screen, 0 if drew
int g3_draw_poly_if_facing(int nv,vertex **pointlist,uint tmap_flags,vec3d *norm,vec3d *pnt)
{
    Assert( G3_count == 1 );

    if (do_facing_check(norm,pointlist,pnt))
        return g3_draw_poly(nv,pointlist,tmap_flags);
    else
        return 255;
}

255 is not -1. Should be (int)-1;

EDIT: It's never called. Never mind.

EDIT2:
There's another case of this on line 702 of 3ddraw.cpp. I can't find a case of where the return value from g3_draw_poly is checked, but it may be worth correcting this anyway.

[portej05]

Kara: Some more patches attached. Most of them are just clarifying cases, but there are one or two in there (such as the change in gropengldraw.cpp) that could affect rendering or gameplay.

Also, around line 1544 of aigoals.cpp there's the line atoi( CTEXT(CDR(CDR(node)))); - shouldn't that be eval_num, given what you told me last night?

[attachment deleted by ninja]