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 |
