Edited descriptions for:

AvoidLargeNumberOfLocalVariablesRule.cs
IDisposableWithDestructorWithoutSuppressFinalize
Rule.cs
AvoidLargeStructureRule.cs
DontIgnoreMethodResultRule.cs
AvoidUnusedParametersRule.cs
AvoidUnsealedConcreteAttributesRule.cs
PreferCharOverloadRule.cs


svn path=/trunk/mono-tools/; revision=140021

8 files changed, 127 insertions, 106 deletions

diff --git a/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeNumberOfLocalVariablesRule.cs b/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeNumberOfLocalVariablesRule.cs
index 929b688d..a1bd50fb 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeNumberOfLocalVariablesRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeNumberOfLocalVariablesRule.cs
@@ -27,6 +27,7 @@
 //
 
 using System;
+using System.ComponentModel;
 
 using Mono.Cecil;
 using Mono.Cecil.Cil;
@@ -36,8 +37,6 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: Can users change the default?
-
 	/// <summary>
 	/// This rule warns when the number of local variables exceed a maximum value (default is
 	/// 64). Having a large amount of local variables makes it hard to generate code that 
@@ -45,13 +44,18 @@ namespace Gendarme.Rules.Performance {
 	/// </summary>
 	/// <remarks>This rule is available since Gendarme 2.0</remarks>
 
-	[Problem ("The number of local variables is too large to allow the JIT to properly allocate registers.")]
+	[Problem ("The number of local variables is so large that the JIT will be unable to properly allocate registers.")]
 	[Solution ("Refactor your code to reduce the number of variables or split the method into several methods.")]
 	[FxCopCompatibility ("Microsoft.Performance", "CA1809:AvoidExcessiveLocals")]
 	public class AvoidLargeNumberOfLocalVariablesRule : Rule, IMethodRule {
 
-		private int max_variables = 64;
+		private const int DefaultMaxVariables = 64;
+		private int max_variables = DefaultMaxVariables;
 
+		/// <summary>The maximum number of local variables which methods may have without a defect being reported.</summary>
+		/// <remarks>Defaults to 64.</remarks>
+		[DefaultValue (DefaultMaxVariables)]
+		[Description ("The maximum number of local variables which methods may have without a defect being reported.")]
 		public int MaximumVariables {
 			get { return max_variables; }
 			set { max_variables = value; }
diff --git a/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeStructureRule.cs b/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeStructureRule.cs
index c532f36b..1ca98e5a 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeStructureRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/AvoidLargeStructureRule.cs
@@ -28,6 +28,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.ComponentModel;
 
 using Mono.Cecil;
 
@@ -36,14 +37,12 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: Can users actually change the default? What is a "calculated field"?
-
 	/// <summary>
 	/// This rule will fire if a value type (struct in C#) is larger than a maximum value
 	/// (16 bytes by default). This is a problem because, unlike reference types, value
 	/// types are bitwise-copied whenever they are assigned to a variable or passed to
 	/// a method. If the type 
-	/// cannot be reduced in size (e.g. by removing calculated fields) then it should be 
+	/// cannot be reduced in size then it should be 
 	/// turned into a reference type (class in C#).
 	/// </summary>
 	/// <example>
@@ -57,7 +56,7 @@ namespace Gendarme.Rules.Performance {
 	/// <example>
 	/// Good example:
 	/// <code>
-	/// public class BigArgb {
+	/// public sealed class BigArgb {
 	///	long a, r, g, b;
 	/// }
 	/// </code>
@@ -68,12 +67,7 @@ namespace Gendarme.Rules.Performance {
 	[Solution ("Try to reduce the struct size or change it into a class.")]
 	public class AvoidLargeStructureRule : Rule, ITypeRule {
 
-		private const int MaximumRecommandedSize = 16;
-
-		private const int MediumSeverityLimit = MaximumRecommandedSize * 2;
-		private const int HighSeverityLimit = MaximumRecommandedSize * 4;
-		private const int CriticalSeverityLimit = MaximumRecommandedSize * 16;
-
+		private const int MaximumRecommendedSize = 16;
 		private const int DefaultPaddingSize = 8;
 
 		// actually we should use IntPtr.Size but that would make the rule
@@ -97,6 +91,33 @@ namespace Gendarme.Rules.Performance {
 			{ Constants.UIntPtr, ReferenceSize },	// on 32 and 64 bits architectures
 		};
 
+		private int max_size = MaximumRecommendedSize;
+		private int medium_severity_limit;
+		private int high_severity_limit;
+		private int critical_severity_limit;
+
+		public override void Initialize (IRunner runner)
+		{
+			base.Initialize (runner);
+			
+			medium_severity_limit = max_size * 2;
+			high_severity_limit = max_size * 4;
+			critical_severity_limit = max_size * 16;
+		}
+		
+		/// <summary>The maximum size structs may be without a defect.</summary>
+		/// <remarks>Defaults to 16 bytes.</remarks>
+		[DefaultValue (MaximumRecommendedSize)]
+		[Description ("The maximum size structs may be without a defect.")]
+		public int MaxSize {
+			get { return max_size; }
+			set {
+				if (value <= 0)
+					throw new ArgumentOutOfRangeException ("MaxSize", "Must be positive");
+				max_size = value;
+			}
+		}
+
 		private static long SizeOfEnum (TypeDefinition type)
 		{
 			foreach (FieldDefinition field in type.Fields) {
@@ -183,16 +204,16 @@ namespace Gendarme.Rules.Performance {
 			// rule applies
 
 			long size = SizeOfStruct (type);
-			if (size <= MaximumRecommandedSize)
+			if (size <= max_size)
 				return RuleResult.Success;
 
 			// here we compute severity based on the actual struct size
 			Severity severity = Severity.Low;
-			if (size > CriticalSeverityLimit)
+			if (size > critical_severity_limit)
 				severity = Severity.Critical;
-			else if (size > HighSeverityLimit)
+			else if (size > high_severity_limit)
 				severity = Severity.High;
-			else if (size > MediumSeverityLimit)
+			else if (size > medium_severity_limit)
 				severity = Severity.Medium;
 
 			string text = String.Format ("Structure size is {0} bytes.", size);
diff --git a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnsealedConcreteAttributesRule.cs b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnsealedConcreteAttributesRule.cs
index 2f04416d..346d8cff 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnsealedConcreteAttributesRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnsealedConcreteAttributesRule.cs
@@ -33,14 +33,11 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: It would be nice to explain in a bit more detail what the performance issues
-	// are.
-
 	/// <summary>
-	/// This rule is used to warn the developer if both unsealed and concrete (not abstract) 
-	/// attribute types are defined in the assembly. If you want other attributes to be able
-	/// to derive from your attribute class, make it <c>abstract</c>. Otherwise, make them
-	/// <c>sealed</c> to improve the performance.
+	/// This rule fires if an attribute is defined which is both concrete (i.e. not abstract)
+	/// and unsealed. This is a performance problem because it means that 
+	/// <c>System.Attribute.GetCustomAttribute</c> has to search the attribute type
+	// hierarchy for derived types. To fix this either seal the type or make it abstract. 
 	/// </summary>
 	/// <example>
 	/// Bad example:
@@ -72,7 +69,7 @@ namespace Gendarme.Rules.Performance {
 	/// </example>
 	/// <remarks>Before Gendarme 2.0 this rule was named AvoidUnsealedAttributesRule.</remarks>
 
-	[Problem ("Due performance issues, concrete attributes should be sealed.")]
+	[Problem ("Because of performance issues, concrete attributes should be sealed.")]
 	[Solution ("Unless you plan to inherit from this attribute you should consider sealing it.")]
 	[FxCopCompatibility ("Microsoft.Performance", "CA1813:AvoidUnsealedAttributes")]
 	public class AvoidUnsealedConcreteAttributesRule : Rule, ITypeRule {
diff --git a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnusedParametersRule.cs b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnusedParametersRule.cs
index d7b5efad..c2dedbeb 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnusedParametersRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnusedParametersRule.cs
@@ -40,11 +40,6 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: Like DoNotIgnoreMethodResultRule this really needs to describe how
-	// to fix the defect in cases where the argument really is not used. (And it can
-	// recommend an Unused type like the one in the DoNotIgnoreMethodResultRule
-	// TODO).
-
 	/// <summary>
 	/// This rule is used to ensure that all parameters in a method signature are being used.
 	/// The rule wont report a defect against the following:
diff --git a/gendarme/rules/Gendarme.Rules.Performance/ChangeLog b/gendarme/rules/Gendarme.Rules.Performance/ChangeLog
index 220f308a..0d9297e6 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Performance/ChangeLog
@@ -1,3 +1,11 @@
+2009-08-16  Jesse Jones  <jesjones@mindspring.com>
+
+	* AvoidLargeNumberOfLocalVariablesRule.cs, 
+	IDisposableWithDestructorWithoutSuppressFinalizeRule, 
+	AvoidLargeStructureRule.cs, DontIgnoreMethodResultRule.cs, 
+	AvoidUnusedParametersRule.cs, AvoidUnsealedConcreteAttributesRule.cs, 
+	PreferCharOverloadRule.cs: Edited descriptions.
+
 2009-07-08  Jesse Jones  <jesjones@mindspring.com>
 
 	* CompareWithStringEmptyEfficientlyRule.cs: Use .NET instead of
diff --git a/gendarme/rules/Gendarme.Rules.Performance/DontIgnoreMethodResultRule.cs b/gendarme/rules/Gendarme.Rules.Performance/DontIgnoreMethodResultRule.cs
index a41e34b9..c8736b3f 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/DontIgnoreMethodResultRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/DontIgnoreMethodResultRule.cs
@@ -40,34 +40,12 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: This is a nice rule, but it's fairly often that people do want to ignore return
-	// values (e.g. from List.Remove) and it's not at all clear to a beginner how best to
-	// do so. You can't cast to void as you can in C and C++ and you can't assign to an
-	// unused local because the compiler will complain. The best approach seems to be
-	// to create a little type like so:
-//	internal static class Unused
-//	{
-//		public static object Value
-//		{
-//			set {}
-//		}
-//	}
-
-	// TODO: Why is the first issue that the summary brings up the expense of allocating
-	// the return value? Maybe this is a problem if the return value is never used but surely
-	// it is a second tier concern. It seems to me that the primary benefit of this rule is
-	// that it points out places where you may be ignoring results without realizing it and
-	// (if you use something like the Unused struct above) you can make it clear to readers
-	// of the code that you are deliberately ignoring the return value.
-
 	/// <summary>
-	/// This rule fires if the result of a method call is not used.
-	/// Since any returned object potentially requires memory allocations this impacts 
-	/// performance. Furthermore this often indicates that the code might not be doing 
-	/// what is expected. This is seen frequently on <c>string</c> where people forget
-	/// that string is an immutable type. There are some special cases, e.g. <c>StringBuilder</c>, where 
-	/// some methods returns the current instance (to chain calls). The rule will ignore 
-	/// those well known cases. 
+	/// This rule fires if a method is called that returns a new instance but that instance
+	/// is not used. This is a performance problem because it is wasteful to create and
+	/// collect objects which are never actually used. It may also indicate a logic problem.
+	/// Note that this rule currently only checks methods within a small number of System
+	/// types.
 	/// </summary>
 	/// <example>
 	/// Bad example:
@@ -75,8 +53,8 @@ namespace Gendarme.Rules.Performance {
 	/// public void GetName ()
 	/// {
 	///	string name = Console.ReadLine ();
-	///	// a new trimmed string is created but never assigned to anything
-	///	// and name itself is unchanged
+	///	// This is a bug: strings are (mostly) immutable so Trim leaves
+	/// 	// name untouched and returns a new string.
 	///	name.Trim ();
 	///	Console.WriteLine ("Name: {0}", name);
 	/// }
diff --git a/gendarme/rules/Gendarme.Rules.Performance/IDisposableWithDestructorWithoutSuppressFinalizeRule.cs b/gendarme/rules/Gendarme.Rules.Performance/IDisposableWithDestructorWithoutSuppressFinalizeRule.cs
index 57236f26..7d813919 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/IDisposableWithDestructorWithoutSuppressFinalizeRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/IDisposableWithDestructorWithoutSuppressFinalizeRule.cs
@@ -38,9 +38,6 @@ using Gendarme.Framework.Rocks;
 
 namespace Gendarme.Rules.Performance {
 
-	// TODO: The good example, at least, should be rewritten to use the Dispose pattern
-	// (i.e. it should use a Dispose(bool) method.
-	
 	/// <summary>
 	/// This rule will fire if a type implements <c>System.IDisposable</c> and has a finalizer
 	/// (called a destructor in C#), but the Dispose method does not call <c>System.GC.SuppressFinalize</c>.
@@ -50,41 +47,61 @@ namespace Gendarme.Rules.Performance {
 	/// <example>
 	/// Bad example:
 	/// <code>
-	/// class Test: SomeClass, IDisposable {
-	///	~Test ()
-	///	{
-	///		if (ptr != IntPtr.Zero) {
-	///			Free (ptr);
-	///		}
-	///	}
-	///	
-	///	public void Dispose ()
-	///	{
-	///		if (ptr != IntPtr.Zero) {
-	///			Free (ptr);
-	///		}
-	///	}
+	/// class BadClass : IDisposable {
+	/// 	~BadClass ()
+	/// 	{
+	/// 		Dispose (false);
+	/// 	}
+	/// 	
+	/// 	public void Dispose ()
+	/// 	{
+	/// 		// GC.SuppressFinalize is missing so the finalizer will be called
+	/// 		// which puts needless extra pressure on the garbage collector.
+	/// 		Dispose (true);
+	/// 	}
+	/// 	
+	/// 	private void Dispose (bool disposing)
+	/// 	{
+	/// 		if (ptr != IntPtr.Zero) {
+	/// 			Free (ptr);
+	/// 			ptr = IntPtr.Zero;
+	/// 		}
+	/// 	}
+	/// 	
+	/// 	[DllImport ("somelib")]
+	/// 	private static extern void Free (IntPtr ptr);
+	/// 	
+	/// 	private IntPtr ptr;
 	/// }
 	/// </code>
 	/// </example>
 	/// <example>
 	/// Good example:
 	/// <code>
-	/// class Test: SomeClass, IDisposable {
-	///	~Test ()
-	///	{
-	///		if (ptr != IntPtr.Zero) {
-	///			Free (ptr);
-	///		}
-	///	}
-	///	
-	///	public void Dispose ()
-	///	{
-	///		if (ptr != IntPtr.Zero) {
-	///			Free (ptr);
-	///		}
-	///		GC.SuppressFinalize (this);
-	///	}
+	/// class GoodClass : IDisposable {
+	/// 	~GoodClass ()
+	/// 	{
+	/// 		Dispose (false);
+	/// 	}
+	/// 	
+	/// 	public void Dispose ()
+	/// 	{
+	/// 		Dispose (true);
+	/// 		GC.SuppressFinalize (this);
+	/// 	}
+	/// 	
+	/// 	private void Dispose (bool disposing)
+	/// 	{
+	/// 		if (ptr != IntPtr.Zero) {
+	/// 			Free (ptr);
+	/// 			ptr = IntPtr.Zero;
+	/// 		}
+	/// 	}
+	/// 	
+	/// 	[DllImport ("somelib")]
+	/// 	private static extern void Free (IntPtr ptr);
+	/// 	
+	/// 	private IntPtr ptr;
 	/// }
 	/// </code>
 	/// </example>
diff --git a/gendarme/rules/Gendarme.Rules.Performance/PreferCharOverloadRule.cs b/gendarme/rules/Gendarme.Rules.Performance/PreferCharOverloadRule.cs
index 3f78a24d..a64c5651 100644
--- a/gendarme/rules/Gendarme.Rules.Performance/PreferCharOverloadRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Performance/PreferCharOverloadRule.cs
@@ -41,23 +41,24 @@ namespace Gendarme.Rules.Performance {
 	// rule request from
 	// https://bugzilla.novell.com/show_bug.cgi?id=406889
 	
-	// TODO: The summary is not as clear as it could be: a lot of people are not
-	// going to know what an ordinal comparison is in this context and it's not
-	// entirely clear what .NET 4.0 is actually changing. It would probably be better
-	// to explain what the rule is looking for, why using char is better (performance)
-	// and then follow up with a second paragraph that points out the subtle differences
-	// between using char and string arguments pre .NET 4.0.
-
 	/// <summary>
 	/// This rule looks for calls to <c>String</c> methods that use <b>String</b>
 	/// parameters when a <c>Char</c> parameter could have been used. Using the
-	/// <c>Char</c>-based method overload (ordinal comparison) is faster than the 
-	/// <c>String</c>-based one (culture-sensitive comparison). 
-	/// Note that .NET 4 will do a breaking change and will use ordinal comparison 
-	/// by default on <c>String</c>. If you need a culture-sensitive comparison 
-	/// use an overload that allow you to specify a <c>StringComparison</c>.
-	/// This rule checks specifically for <c>IndexOf</c>, <c>LastIndexOf</c> and
-	/// <c>Replace</c> to ensure there is a performance gain (other methods could vary).
+	/// <c>Char</c> overload is preferred because it will be faster. 
+	///
+	/// Note, however, that this may result in subtly different behavior on versions of
+	/// .NET before 4.0: the string overloads do a culture based comparison using
+	/// <c>CultureInfo.CurrentCulture</c> and the char methods do an ordinal
+	/// comparison (a simple compare of the character values). This can result in
+	/// a change of behavior (for example the two can produce different results when
+	/// precomposed characters are used). If this is important it is best to use an
+	/// overload that allows StringComparison or CultureInfo to be explicitly specified
+	/// see [http://msdn.microsoft.com/en-us/library/ms973919.aspx#stringsinnet20_topic4] 
+	/// for more details.
+	///
+	/// With .NET 4.0 <c>String</c>'s behavior will change and the various methods
+	/// will be made more consistent. In particular the comparison methods will be changed
+	/// so that they all default to doing an ordinal comparison.
 	/// </summary>
 	/// <example>
 	/// Bad example: