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 <[EMAIL PROTECTED]> 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@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list > > >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list