[Mono-dev] Pending patches

[Paolo Molaro]

On 03/14/08 Kornél Pál wrote:
> I didn't receive any comments or approval on the following patches:

Sorry for the delay, here are a few quick comments:

> http://lists.ximian.com/pipermail/mono-devel-list/2008-March/027166.html

The changes look fine to me, but someone using windows will have to
comment on them.

> http://lists.ximian.com/pipermail/mono-devel-list/2008-February/027044.html

> Index: mcs/class/corlib/System.Text/InternalEncoding.cs
> ===================================================================
> --- mcs/class/corlib/System.Text/InternalEncoding.cs	(revision 0)
> +++ mcs/class/corlib/System.Text/InternalEncoding.cs	(revision 0)

I don't like the introduction of this class and it doesn't seem to have
much purpose. It also seems orthogonal to the rest of the changes, so
please drop this change from the patch: you should try to introduce one
change at a time instead of conflating several different unrelated
changes in one patch.

> Index: mcs/class/corlib/System.Text/UTF8Encoding.cs
> ===================================================================
> --- mcs/class/corlib/System.Text/UTF8Encoding.cs	(revision 96483)
> +++ mcs/class/corlib/System.Text/UTF8Encoding.cs	(working copy)
> @@ -40,6 +40,8 @@
>  	// Magic number used by Windows for UTF-8.
>  	internal const int UTF8_CODE_PAGE = 65001;
>  
> +	private static readonly Type internalType = typeof (UTF8Encoding);
> +

There is no need for caching typeof (), it will just waste memory.

>  	// Internal state.
>  	private bool emitIdentifier;
>  #if !NET_2_0
> @@ -73,6 +75,46 @@
>  		windows_code_page = UnicodeEncoding.UNICODE_CODE_PAGE;
>  	}
>  
> +	internal override bool IsInternal {
> +		get { return this.GetType () == internalType; }
> +	}
> +
> +	internal override unsafe int GetByteCountImpl (char* chars, int charCount)
> +	{
> +		char leftOver = '\0';
> +		return InternalGetByteCount (chars, charCount, ref leftOver, true);

I would much prefer to see the argument validation close to the unsafe
code and I don't particularly like the use of these internal virtual
wrapper methods.
The code duplication can be avoided by using simple static validation
methods like (only for the cases when more that two lines of checks are
needed):

	static void ValidateGetBytes (...same arguments as GetBytes ()...) {
	}

Basically using virtual+unsafe should be severely limited in our managed
assemblies or it's going to increase the cost of security checks
significantly.

> Index: mcs/class/corlib/System.Text/Encoding.cs
> ===================================================================
> --- mcs/class/corlib/System.Text/Encoding.cs	(revision 96483)
> +++ mcs/class/corlib/System.Text/Encoding.cs	(working copy)
> +	internal unsafe int GetBytesInternal (string s, int charIndex, int charCount, byte [] bytes, int byteIndex)
> +	{
> +		if (s == null)
> +			throw new ArgumentNullException ("s");
> +		if (bytes == null)
> +			throw new ArgumentNullException ("bytes");
>  		if (charIndex < 0 || charIndex > s.Length)
>  			throw new ArgumentOutOfRangeException ("charIndex", _("ArgRange_Array"));
> -		if (charCount < 0 || charIndex + charCount > s.Length)
> +		if (charCount < 0 || charIndex > (s.Length - charCount))

These fixes should be in their own separate patch instead of being
hidden in a large change.

> Index: mcs/class/corlib/System.Text/UnicodeEncoding.cs
> ===================================================================
> --- mcs/class/corlib/System.Text/UnicodeEncoding.cs	(revision 96483)
> +++ mcs/class/corlib/System.Text/UnicodeEncoding.cs	(working copy)
> @@ -97,27 +99,19 @@
>  		windows_code_page = UNICODE_CODE_PAGE;
>  	}
>  
> +	internal override bool IsInternal {
> +		get { return this.GetType () == internalType; }
> +	}
> +
>  	// Get the number of bytes needed to encode a character buffer.
>  	public override int GetByteCount (char[] chars, int index, int count)
>  	{
> -		if (chars == null) {
> -			throw new ArgumentNullException ("chars");
> -		}
> -		if (index < 0 || index > chars.Length) {
> -			throw new ArgumentOutOfRangeException ("index", _("ArgRange_Array"));
> -		}
> -		if (count < 0 || count > (chars.Length - index)) {
> -			throw new ArgumentOutOfRangeException ("count", _("ArgRange_Array"));
> -		}
> -		return count * 2;
> +		return GetByteCountInternal (chars, index, count);

For these simple cases I think that hiding count*2 inside
GetByteCountInternal() doesn't help code readability or any other
property of the code. This would look much better if it was instead like
this:

  	public override int GetByteCount (char[] chars, int index, int count)
  	{
		ValidateGetByteCount (chars, index, count);

		return count * 2;
  	}

>  	public override int GetByteCount (String s)
>  	{
> -		if (s == null) {
> -			throw new ArgumentNullException ("s");
> -		}
> -		return s.Length * 2;
> +		return GetByteCountInternal (s);
>  	}

Same here, when the validation is so simple there is no point in
moving it out of line.
The other files have similar issues.

What I suggest is the following:
1) a small patch with the bugfixes
2) another patch that introduces the validation methods
and reduces the code duplication
3) separate patches for each encoding with the optimizations

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better