[Mono-dev] Patch for String.cs

[Andreas Nahr]

Sorry, but after working soooo long on this (series of) patches you somehow
assume other people know the story, too. Obviously that was a misconception
on my part ;)

Just as a brief info: This patch is a part of a big patch that has been
discussed for two? years on and off. And it was accepted by Miguel some time
(months?) ago. However there were some regressions so it had to be reverted.
The idea now is/was to commit smaller parts of it to get a better overview
of where problems occurred. This is the 3rd last of those parts (the others
are already committed). However as it affects really core parts I thought it
would be better for some more people to try, especially as this was one of
the two parts that actually caused the regressions back then (the cause has
hopefully been fixed by now).

Other parts in text.

> <generic rant/>
> 
> To make it easier (and this almost always means faster to) to review
> please keep an *updated* history with your patch (not everyone
> remembers
> every email he reads, it's easier for googling later and anyway it's
> likely some things have changed since then). Important things (from the
> reviewer point of view) is explaining:
> 
> * Why the patch exists ?
>         * to fix things ?

Partly. It is part of the bigger string patch. It restructures string and
replaces C implementation where left with managed code. It also concentrates
the relevant code that is currently spread out though the corlib classlib
and the runtime in string.cs. As part (sideeffect) of this it also fixes
bugs in the current implementation. (String is/was in a really miserable
state, especially when it comes to the 2.0 profile)

>                 * where are the bug number(s) ?
>                 * where are the unit tests ?

Unit tests are already commited. I've added quite a load of them ;). Some of
them are currently disabled because they fail against current mono. I'll add
a patch to this post that removes the comment status from those.

>                 * NOTE: you can save time by including this, inside the
>                 patch, into the **required** ChangeLog entry
> 
> * Why there is no new unit test part of the patch
> 	* even if you're adding new exceptions

See above

> * Why you introduce unsafe code
>         * performance ?
>                 * where's the data ? and I don't mean just numbers!
>                 where's the code ? how was it measured? on which
>                 architecture ?

This has been discussed and posted to this list a long time ago.

>         * moving stuff from icalls ?
>                 * what will be removed ? i.e. do we get more or less
>                 unsafe (C or C#) code to review ?

Well basically it replaces unmanaged unsafe C with managed unsafe C# or
managed safe C#. Once all parts are committed string-icall.c is mostly
obsolete. The complete amount of unsafe code will be somewhat less because
only parts are actually unsafe. On the other hand some new implementations
are far more advanced than the old C code and thus a little bit longer.

> Also *clearly* state if you have executed the existing unit tests (and
> for each profile you did). For highly used code (like String) please
> state if you did a full bootstrap and execute unit tests (for all
> assemblies, all profiles). Without answers it's fairly likely most of
> them will be interpreted as a *negative*, so a job for the reviewer.

I did test them on my machine in a bootstap and corlib unittest as far as
possible with my setup (x86, Cygwin).
 
> > Already mailed this to Miguel a few days ago, but did not get any
> feedback
> > yet.
> 
> He's likely busy with a lot of other things. Anyway mono-devel is the
> right place to post patches.

I was somewhat unsure if he got the mail at all because there seemed to be
some problem with our mail-contact shortly before that as he couldn't reach
me.

> Note that most people able to review this already have a TODO plan for
> the next week and large patches often don't fit within them. Even less
> if the gains are unclear (e.g. no bug fixed) or the potential problems
> high (e.g. is it tested ?).
> 
> Keep in mind that all the previous unanswered question are something
> that a reviewer would need to answer by himself (i.e. the more
> questions
> the more time it takes, the less likely someone has the time to review
> it).
> 
> Anyway that's my own story on why some patches takes longer than others
> to be reviewed - or at least how you can help make it faster ;-)
> 
> Thanks for the patch :)
> Sebastien
-------------- next part --------------
A non-text attachment was scrubbed...
Name: StringTest.patch
Type: application/octet-stream
Size: 1030 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080528/cbfad8bd/attachment.obj