Bug 3993 - Bug in implementation of native function strtok.
Bug in implementation of native function strtok.
Status: RESOLVED FIXED
Product: AMX Mod X
Classification: Unclassified
Component: Core
trunk
PC All
: P3 normal
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-06 12:25 PDT by Lev2001
Modified: 2013-08-02 03:16 PDT (History)
5 users (show)

See Also:
dvander: blocking‑1.8.2+


Attachments
Patch (460 bytes, patch)
2009-09-14 21:19 PDT, Lev2001
no flags Details | Diff | Splinter Review
strtok2 (4.73 KB, patch)
2013-07-31 13:28 PDT, Valentin Grünbacher [:Nextra]
hv.contact: review+
Details | Diff | Splinter Review

Description Lev2001 2009-09-06 12:25:51 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];
        }
    }
Comment 1 David Anderson [:dvander] 2009-09-06 22:03:47 PDT
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).
Comment 2 Lev2001 2009-09-14 21:19:58 PDT
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. :)
Comment 3 Lev2001 2009-09-15 02:02:53 PDT
BTW In first post I make some mistakes in replacement code. So use diff information - it should be ok.
Comment 4 Reuben Morais 2010-12-20 11:24:09 PST
This one needs check-in.
Comment 5 David Anderson [:dvander] 2013-02-13 01:13:15 PST
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().
Comment 6 Vincent Herbet [:Arkshine] 2013-02-13 15:55:28 PST
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 " ?
Comment 7 Valentin Grünbacher [:Nextra] 2013-07-31 13:28:17 PDT
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 8 Vincent Herbet [:Arkshine] 2013-07-31 14:16:46 PDT
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.
Comment 9 AM Bugzilla Bot 2013-08-02 03:04:19 PDT
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/4dfef946029a
Changelog:
	Add strtok2 which fixes a trim issue with strtok (bug 3993, r=arkshine)

Note You need to log in before you can comment on or make changes to this bug.