Bugzilla – Bug 3993
Bug in implementation of native function strtok.
Last modified: 2013-08-02 03:16:09 PDT
By declaration: " strtok - Breaks a string into two halves, by token. Syntax: strtok ( const text[], Left[], leftLen, Right[], rightLen, token=' ', trimSpaces=0 ) If you use trimSpaces, all spaces are trimmed from Left. " I.e. function should break string by token and then trim whitespaces from left part. So in exampple of: strtok("asd qwe |zxc", ...,...,...,...,'|",0); It should give: left: "asd qwe " right: "zxc" And in this exmaple: strtok("asd qwe |zxc", ...,...,...,...,'|",1); It should give this: left: "asd qwe" right: "zxc" But in second example it gives (current behaviour): left: "asd" right: "qwe|zxc" Part of source code with bug: for (i = 0; i < (unsigned int)len; i++) { if (trim && !done_flag) { if (isspace(string[i])) { while (isspace(string[++i])); done_flag = true; } } if (!done_flag && string[i] == token) { done_flag = true; i++; } if (done_flag) { right[right_pos++] = string[i]; } else { left[left_pos++] = string[i]; } } should be rewritten such (just a suggetion): if (trim) while (isspace(string[i++])); int space_pos = 0; for (; i < (unsigned int)len; i++) { if (trim && !done_flag) { if (isspace(string[i])) space_pos = left_pos; else if (string[i] != token) space_pos = 0; } if (!done_flag && string[i] == token) { if (space_pos) left_pos = space_pos; done_flag = true; i++; } if (done_flag) { right[right_pos++] = string[i]; } else { left[left_pos++] = string[i]; } }
Please attach source code changes as a diff (subversion tools can generate one, or via the command line with "svn diff" or "diff" on Unix-like systems).
Created attachment 1709 [details] [review] Patch This patch should fix described bug. I strongly suggest to test it in place, because I tested it only in my mind. :)
BTW In first post I make some mistakes in replacement code. So use diff information - it should be ok.
This one needs check-in.
Going to sit on this for 1.8.2, as it looks like it has potential compat issues. Might be best to fork a strtok2().
Fair. Such native is used often, and we can expect plugin using it with additional code to fix wrong behavior, and by changing how works the native, it could break plugins for sure. Safer to fork the native. David, I've somehow a doubt : is the expected result, using trimSpaces option, would be to trim white-spaces from left *only* for the left part (and not right too) ? I.e. : " AAAA | BBBB " should be "AAAA " and, " BBBB " or "BBBB " ?
Created attachment 3472 [details] [review] strtok2 New native strtok2 with fixed and expanded functionality. - Trim parameter allows for more configuration of trimming behavior with a set of flags. - strtok2 returns position where token was found. -1 indicates that token was not found. Hopefully I have not commited too many code style sins :)
Comment on attachment 3472 [details] [review] strtok2 Review of attachment 3472 [details] [review]: ----------------------------------------------------------------- Don't worry, it's easy to review as it is and it looks good. I also know you have tested it properly. The idea about flags is nice even if it may appear clumsy, but I doubt we can do something better. Thanks for this.
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/4dfef946029a Changelog: Add strtok2 which fixes a trim issue with strtok (bug 3993, r=arkshine)