View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003069 | FSSCP | tables | public | 2014-06-29 02:30 | 2014-07-19 13:09 |
Reporter | MjnMixael | Assigned To | MageKing17 | ||
Priority | normal | Severity | crash | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | PC | OS | Windows | OS Version | Win7 |
Product Version | 3.7.2 | ||||
Target Version | 3.7.2 | ||||
Summary | 0003069: ASSERTION when help.tbl has errors | ||||
Description | This should be an Error that is actually a little helpful.. handled in the same was as other tables' errors. | ||||
Steps To Reproduce | write +resolution instead of +resolutions in help.tbl. | ||||
Tags | No tags attached. | ||||
|
contexthelp.cpp.patch (481 bytes)
Index: code/gamehelp/contexthelp.cpp =================================================================== --- code/gamehelp/contexthelp.cpp (revision 10846) +++ code/gamehelp/contexthelp.cpp (working copy) @@ -482,7 +482,7 @@ } else { // help.tbl is corrupt - Assert(0); + Error(LOCATION, "help.tbl appears to be corrupt; expected [+pline], [+text], [+right_bracket], [+left_bracket], or [$end], found [%.32s] instead.\n", next_tokens()); } // end if |
|
contexthelp2.patch (6,175 bytes)
Index: code/gamehelp/contexthelp.cpp =================================================================== --- code/gamehelp/contexthelp.cpp (revision 10855) +++ code/gamehelp/contexthelp.cpp (working copy) @@ -411,11 +411,13 @@ help_overlaylist[overlay_id].lbracketlist.push_back(lbracket_temp); } + int type; // read in all elements for this overlay - while (!(optional_string("$end"))) { + while ((type = required_string_5("+pline", "+text", "+right_bracket", "+left_bracket", "$end")) != 4) { // Doing it this way means an error lists "$end" at the end, which seems appropriate. -MageKing17 - if (optional_string("+pline")) { - + switch (type) { + case 0: // +pline + required_string("+pline"); currcount = help_overlaylist[overlay_id].plinecount; int a, b; // temp vars to read in int before cast to float; @@ -422,9 +424,9 @@ // read number of pline vertices stuff_int(&vtxcount); // get vertex coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].plinelist.at(i).push_back(pline_temp2); - for (j=0; j<vtxcount; j++) { + for (j = 0; j < vtxcount; j++) { help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtx.push_back(vec3d_temp); help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtxcount = vtxcount; stuff_int(&a); @@ -436,13 +438,13 @@ } help_overlaylist[overlay_id].plinecount++; - - } else if (optional_string("+text")) { - + break; + case 1: // +text + required_string("+text"); currcount = help_overlaylist[overlay_id].textcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].textlist.at(i).push_back(text_temp2); stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).y_coord)); @@ -453,13 +455,13 @@ help_overlaylist[overlay_id].textlist.at(0).at(currcount).string = vm_strdup(buf); help_overlaylist[overlay_id].textcount++; - - } else if (optional_string("+right_bracket")) { - + break; + case 2: // +right_bracket + required_string("+right_bracket"); currcount = help_overlaylist[overlay_id].rbracketcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].rbracketlist.at(i).push_back(rbracket_temp2); stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).y_coord)); @@ -466,13 +468,13 @@ } help_overlaylist[overlay_id].rbracketcount++; - - } else if (optional_string("+left_bracket")) { - + break; + case 3: // +left_bracket + required_string("+left_bracket"); currcount = help_overlaylist[overlay_id].lbracketcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].lbracketlist.at(i).push_back(lbracket_temp2); stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).y_coord)); @@ -479,14 +481,17 @@ } help_overlaylist[overlay_id].lbracketcount++; - - } else { - // help.tbl is corrupt - Assert(0); - - } // end if - + break; + case -1: + // -noparseerrors is set + break; + case 4: // $end + default: + Assertion(false, "This should never happen.\n"); + break; + } } // end while + required_string("$end"); } // end while // close localization Index: code/parse/parselo.cpp =================================================================== --- code/parse/parselo.cpp (revision 10855) +++ code/parse/parselo.cpp (working copy) @@ -740,6 +740,42 @@ return -1; } +int required_string_5(char *str1, char *str2, char *str3, char *str4, char *str5) +{ + int count = 0; + + ignore_white_space(); + + while (count < RS_MAX_TRIES) { + if (strnicmp(str1, Mp, strlen(str1)) == 0) { + // Mp += strlen(str1); + diag_printf("Found required string [%s]\n", token_found = str1); + return 0; + } else if (strnicmp(str2, Mp, strlen(str2)) == 0) { + // Mp += strlen(str2); + diag_printf("Found required string [%s]\n", token_found = str2); + return 1; + } else if (strnicmp(str3, Mp, strlen(str3)) == 0) { + diag_printf("Found required string [%s]\n", token_found = str3); + return 2; + } else if (strnicmp(str4, Mp, strlen(str4)) == 0) { + diag_printf("Found required string [%s]\n", token_found = str4); + return 3; + } else if (strnicmp(str5, Mp, strlen(str5)) == 0) { + diag_printf("Found required string [%s]\n", token_found = str5); + return 4; + } + + error_display(1, "Required token = [%s], [%s], [%s], [%s], or [%s], found [%.32s].\n", str1, str2, str3, str4, str5, next_tokens()); + + advance_to_eoln(NULL); + ignore_white_space(); + count++; + } + + return -1; +} + int required_string_either_fred(char *str1, char *str2) { ignore_white_space(); Index: code/parse/parselo.h =================================================================== --- code/parse/parselo.h (revision 10855) +++ code/parse/parselo.h (working copy) @@ -113,6 +113,7 @@ extern int required_string_either(char *str1, char *str2); extern int required_string_3(char *str1, char *str2, char *str3); extern int required_string_4(char *str1, char *str2, char *str3, char *str4); +extern int required_string_5(char *str1, char *str2, char *str3, char *str4, char *str5); // stuff extern void copy_to_eoln(char *outstr, char *more_terminators, char *instr, int max); |
|
Attached three patches with varying levels of impact. The first patch just changes "Assert(0)" into a descriptive Error(). The second patch rewrites the logic, adding required_string_5() to match required_string_3() and required_string_4(). The third patch is the same as the second, but instead of the silly required_string_5(), it uses a new required_string_any_of() with a variable number of arguments that can be used as a drop-in replacement for required_string_3() or required_string_4() in the future. Most of it was written by ngld during a conversation on #scp as we were discussing this bug. |
|
contexthelp3.patch (6,320 bytes)
Index: code/gamehelp/contexthelp.cpp =================================================================== --- code/gamehelp/contexthelp.cpp (revision 10856) +++ code/gamehelp/contexthelp.cpp (working copy) @@ -411,11 +411,13 @@ help_overlaylist[overlay_id].lbracketlist.push_back(lbracket_temp); } + int type; // read in all elements for this overlay - while (!(optional_string("$end"))) { + while ((type = required_string_one_of(5, "+pline", "+text", "+right_bracket", "+left_bracket", "$end")) != 4) { // Doing it this way means an error lists "$end" at the end, which seems appropriate. -MageKing17 - if (optional_string("+pline")) { - + switch (type) { + case 0: // +pline + required_string("+pline"); currcount = help_overlaylist[overlay_id].plinecount; int a, b; // temp vars to read in int before cast to float; @@ -422,9 +424,9 @@ // read number of pline vertices stuff_int(&vtxcount); // get vertex coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].plinelist.at(i).push_back(pline_temp2); - for (j=0; j<vtxcount; j++) { + for (j = 0; j < vtxcount; j++) { help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtx.push_back(vec3d_temp); help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtxcount = vtxcount; stuff_int(&a); @@ -436,13 +438,13 @@ } help_overlaylist[overlay_id].plinecount++; - - } else if (optional_string("+text")) { - + break; + case 1: // +text + required_string("+text"); currcount = help_overlaylist[overlay_id].textcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].textlist.at(i).push_back(text_temp2); stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).y_coord)); @@ -453,13 +455,13 @@ help_overlaylist[overlay_id].textlist.at(0).at(currcount).string = vm_strdup(buf); help_overlaylist[overlay_id].textcount++; - - } else if (optional_string("+right_bracket")) { - + break; + case 2: // +right_bracket + required_string("+right_bracket"); currcount = help_overlaylist[overlay_id].rbracketcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].rbracketlist.at(i).push_back(rbracket_temp2); stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).y_coord)); @@ -466,13 +468,13 @@ } help_overlaylist[overlay_id].rbracketcount++; - - } else if (optional_string("+left_bracket")) { - + break; + case 3: // +left_bracket + required_string("+left_bracket"); currcount = help_overlaylist[overlay_id].lbracketcount; // get coordinates for each resolution - for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) { + for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) { help_overlaylist[overlay_id].lbracketlist.at(i).push_back(lbracket_temp2); stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).x_coord)); stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).y_coord)); @@ -479,14 +481,17 @@ } help_overlaylist[overlay_id].lbracketcount++; - - } else { - // help.tbl is corrupt - Assert(0); - - } // end if - + break; + case -1: + // -noparseerrors is set + break; + case 4: // $end + default: + Assertion(false, "This should never happen.\n"); + break; + } } // end while + required_string("$end"); } // end while // close localization Index: code/parse/parselo.cpp =================================================================== --- code/parse/parselo.cpp (revision 10856) +++ code/parse/parselo.cpp (working copy) @@ -740,6 +740,56 @@ return -1; } +// Generic version of old required_string_3 and required_string_4; written by ngld, with some tweaks by MageKing17 +int required_string_one_of(int arg_count, ...) +{ + Assertion(arg_count > 0, "required_string_one_of() called with arg_count of %d; get a coder!\n", arg_count); + int count = 0; + int idx; + char *expected; + SCP_string message = ""; + va_list vl; + + ignore_white_space(); + + while (count < RS_MAX_TRIES) { + va_start(vl, arg_count); + for (idx = 0; idx < arg_count; idx++) { + expected = va_arg(vl, char*); + if (strnicmp(expected, Mp, strlen(expected)) == 0) { + diag_printf("Found required string [%s]", token_found = expected); + return idx; + } + } + va_end(vl); + + if (!message.compare("")) { + va_start(vl, arg_count); + message = "Required token = "; + for (idx = 0; idx < arg_count; idx++) { + message += "["; + message += va_arg(vl, char*); + message += "]"; + if (arg_count == 2 && idx == 0) { + message += " or "; + } else if (idx == arg_count - 2) { + message += ", or "; + } else if (idx < arg_count - 2) { + message += ", "; + } + } + va_end(vl); + } + + error_display(1, "%s, found [%.32s]\n", message.c_str(), next_tokens()); + advance_to_eoln(NULL); + ignore_white_space(); + count++; + } + + return -1; +} + int required_string_either_fred(char *str1, char *str2) { ignore_white_space(); Index: code/parse/parselo.h =================================================================== --- code/parse/parselo.h (revision 10856) +++ code/parse/parselo.h (working copy) @@ -113,6 +113,7 @@ extern int required_string_either(char *str1, char *str2); extern int required_string_3(char *str1, char *str2, char *str3); extern int required_string_4(char *str1, char *str2, char *str3, char *str4); +extern int required_string_one_of(int arg_count, ...); // stuff extern void copy_to_eoln(char *outstr, char *more_terminators, char *instr, int max); |
|
In the third patch, the message variable is not assigned anything, so the "if (!message.compare(""))" block will never trigger. |
|
It's declared with 'SCP_string message = "";' I did test all three patches, you know. ;) |
|
What I mean is that it isn't modified after its initialization. It's a logic error, not a crash. :) |
|
If it wasn't modified, my test wouldn't have worked. '!message.compare("")' is checking to see if message is not different from "", which it isn't at first, so the block executes. |
|
Ack. Never mind what I said; I forgot that .compare() works like strcmp() and returns a 0 if the strings match. >.< |
|
Fix committed in rev 10925 |
Date Modified | Username | Field | Change |
---|---|---|---|
2014-06-29 02:30 | MjnMixael | New Issue | |
2014-06-29 02:30 | MjnMixael | Status | new => assigned |
2014-06-29 02:30 | MjnMixael | Assigned To | => MageKing17 |
2014-06-29 05:05 | MageKing17 | File Added: contexthelp.cpp.patch | |
2014-06-29 05:06 | MageKing17 | File Added: contexthelp2.patch | |
2014-06-29 05:06 | MageKing17 | File Added: contexthelp3.patch | |
2014-06-29 05:11 | MageKing17 | Note Added: 0015932 | |
2014-06-29 05:11 | MageKing17 | Status | assigned => code review |
2014-06-29 16:47 | MageKing17 | File Deleted: contexthelp3.patch | |
2014-06-29 16:47 | MageKing17 | File Added: contexthelp3.patch | |
2014-06-29 17:42 | MageKing17 | File Deleted: contexthelp3.patch | |
2014-06-29 17:42 | MageKing17 | File Added: contexthelp3.patch | |
2014-06-29 17:55 | MageKing17 | File Deleted: contexthelp3.patch | |
2014-06-29 17:56 | MageKing17 | File Added: contexthelp3.patch | |
2014-06-29 17:57 | MageKing17 | File Deleted: contexthelp3.patch | |
2014-06-29 17:57 | MageKing17 | File Added: contexthelp3.patch | |
2014-06-29 22:58 | Goober5000 | Note Added: 0015936 | |
2014-06-29 23:03 | MageKing17 | Note Added: 0015937 | |
2014-06-29 23:07 | Goober5000 | Note Added: 0015938 | |
2014-06-29 23:12 | MageKing17 | Note Added: 0015940 | |
2014-06-30 02:59 | Goober5000 | Note Added: 0015943 | |
2014-07-19 13:09 | The_E | Note Added: 0016095 | |
2014-07-19 13:09 | The_E | Status | code review => resolved |
2014-07-19 13:09 | The_E | Resolution | open => fixed |