[Mono-dev] [PATCH] String.Split() broken behavior

Rodrigo Kumpera kumpera at gmail.com
Fri Oct 5 22:51:27 EDT 2007

Hi Tyler,

I some coments about our patch, some are just minor details like formating
issues. You can read about our coding guidelines in

For the options it's a good idea to create an enum in the c side. "options &
SPLIT_OPTIONS_REMOVE_EMPTY_ENTRIES", or something alika, is way better than
"options & 0x01" - which uses some sort of magical number. Take a look at
the GenericParameterAttributes enum in object-internals.h.

+            if (string_icall_is_in_array(separator, arrsize, src[i])) {
+                splitsize++;

Please add a space after function name and array index, it should be
something like:
+            if (string_icall_is_in_array (separator, arrsize, src [i])) {
+                splitsize++;

+                if (lastpos == 1)
+                {
+                    splitsize++;
+                }

Avoid braces for single line ifs.

About fixing the broken code, we are going to need new unit tests for all
this code. You could sent the patch for
mcs/class/corlib/Test/System/StringTest.cs too ;)


On 10/5/07, Tyler Larson <mono-devel at tlarson.com> wrote:
> Hey Mono team:
> This is my first time contributing, so please bear with me if I do
> something dumb.
> I've provided a patch for incorrect behavior in the String.Split()
> function. In particular, String.Split(char[],int,StringSplitOptions)
> behaves incorrectly when instructed to remove empty entries while also
> limiting the size of the resultant array.
> Take for example, the following code:
> ",a,,b,c".Split(new char[]{','},3,StringSplitOptions.RemoveEmptyEntries);
> The existing implementation split the string into the following 3
> components:  {[], [], [a,,b,c]}
> Then it scans the array and removes all empty entries, returning simply
> {[a,,b,c]}
> The correct behavior would be to remove empty entries while the string
> was being split, and always return the maximum number of elements
> possible. The *correct* result to the preceding example would be:
> {[a],[b],[c]}
> In order to do this correctly and efficiently, the InternalSplit
> function had to be modified to be able to remove empty entries during
> the split procedure; this included changing the signature to accept an
> "options" parameter.
> This patch also removes the need for further optimization of the
> String.Split() call.
> I'm a bit unsure about the coding style required; the .c file seemed to
> be a bit of a combination of a few coding styles all together. Also, the
> new InternalSplit function in this patch contains the line:
> remempty = options & 0x01;
> where 0x01 refers to the StringSplitOptions.RemoveEmptyEntries flag. I'm
> sure there's a "better" way of indicating this; either by referencing
> the enum somehow or with a #define somewhere. I don't know how you do
> that sort of thing here.
> The patch, as provided, DOES fix the broken code and works without any
> trouble, but I would appreciate it if someone more familiar with the
> Mono project would go over the changes and bring those style bits a bit
> more into conformance with what is expected.
> Cheers
> -Tyler Larson
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20071005/4a82b22d/attachment.html 

More information about the Mono-devel-list mailing list