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

[Rodrigo Kumpera]

Hi Tyler,

I some coments about our patch, some are just minor details like formating
issues. You can read about our coding guidelines in
http://www.mono-project.com/Coding_Guidelines

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 ;)


Cheers,
Rodrigo

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