diff options
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: |