[Mono-dev] Patch for String.cs

[Sebastien Pouliot]

Hello Andreas,

On Wed, 2008-05-28 at 10:24 +0200, Andreas Nahr wrote:
> Attached are two versions of a patch for String.
> The one is the pure patch, the other also cleans up some unused stuff in
> related classes.

<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 ? 
                * where are the bug number(s) ?
                * where are the unit tests ?
                * 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

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

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.

> 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. 

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