From e6105e8a0fd9acd75de25aafba597e3fe8bbe291 Mon Sep 17 00:00:00 2001 From: Nolan Lum Date: Sun, 28 Nov 2010 22:53:06 -0500 Subject: [PATCH] Revise code as per feedback. Add additional test cases and revise rule code. --- .../AvoidUnnecessaryOverridesRule.cs | 46 ++++++++++++--- .../Test/AvoidUnnecessaryOverridesTest.cs | 59 +++++++++++++++---- 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnnecessaryOverridesRule.cs b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnnecessaryOverridesRule.cs index 3c79f3e20..720cc1e4b 100644 --- a/gendarme/rules/Gendarme.Rules.Performance/AvoidUnnecessaryOverridesRule.cs +++ b/gendarme/rules/Gendarme.Rules.Performance/AvoidUnnecessaryOverridesRule.cs @@ -42,25 +42,34 @@ namespace Gendarme.Rules.Performance { /// /// Bad example: /// - /// public override string ToString() + /// public override string ToString () /// { - /// return base.ToString(); + /// return base.ToString (); /// } /// /// /// - /// Good example: + /// Good example (1): /// /// [FileIOPermission (SecurityAction.Demand, @"c:\dir\file")] - /// public override string ToString() + /// public override string ToString () /// { - /// return base.ToString(); + /// return base.ToString (); /// } /// /// + /// + /// Good example (2): + /// + /// /*public override string ToString () + /// { + /// return base.ToString (); + /// }*/ + /// + /// [Problem ("This override of a base class method is unnecessary.")] - [Solution ("Remove the override method.")] + [Solution ("Remove the override method or extend the functionality of the method.")] public class AvoidUnnecessaryOverridesRule : Rule, ITypeRule { public RuleResult CheckType(TypeDefinition type) @@ -88,9 +97,8 @@ public RuleResult CheckType(TypeDefinition type) while (i < instrs.Count && (instrs[i].Is (Code.Nop) || instrs[i].IsLoadArgument ())) i++; - // If the next instruction is not a call we are good. (Relatively certain - // calls to the base class cannot be virtual.) - if (!instrs[i].Is (Code.Call)) + // If the next instruction is not a call we are good. + if (!instrs[i].Is (Code.Call) && !instrs[i].Is(Code.Call)) continue; // Check to make sure the call is to the base class, and the same method name... MethodReference mr = instrs[i].Operand as MethodReference; @@ -102,6 +110,26 @@ public RuleResult CheckType(TypeDefinition type) continue; if (mr.Name != method.Name) continue; + // Account for calling overrides. + if (mr.HasParameters != method.HasParameters) + continue; + // Check the parameter count and type. + if (mr.Parameters.Count != method.Parameters.Count) + continue; + // Shallow check for equality. + bool sameParam = true; + for (int o = 0; o < mr.Parameters.Count; o++) { + var p1 = mr.Parameters[o]; + var p2 = method.Parameters[o]; + sameParam = true; + sameParam &= p1.HasDefault == p2.HasDefault; + sameParam &= p1.HasFieldMarshal == p2.HasFieldMarshal; + sameParam &= p1.IsOptional == p2.IsOptional; + sameParam &= p1.IsOut == p2.IsOut; + sameParam &= p1.ParameterType == p2.ParameterType; + if (!sameParam) break; + } + if (!sameParam) continue; i++; // If the return type is void, all we should have is nop and return. diff --git a/gendarme/rules/Gendarme.Rules.Performance/Test/AvoidUnnecessaryOverridesTest.cs b/gendarme/rules/Gendarme.Rules.Performance/Test/AvoidUnnecessaryOverridesTest.cs index fb4082cd2..b199e3a12 100644 --- a/gendarme/rules/Gendarme.Rules.Performance/Test/AvoidUnnecessaryOverridesTest.cs +++ b/gendarme/rules/Gendarme.Rules.Performance/Test/AvoidUnnecessaryOverridesTest.cs @@ -35,6 +35,7 @@ using Test.Rules.Definitions; using Test.Rules.Fixtures; +using Test.Rules.Helpers; namespace Tests.Rules.Performance { @@ -42,67 +43,101 @@ namespace Tests.Rules.Performance { public class AvoidUnnecessaryOverridesTest : TypeRuleTestFixture { private class TestBaseClass { - public virtual string DoSomething(string s) + public virtual string DoSomething (string s) { return s; } + public virtual string DoSomething () + { + return ":D"; + } } private class TestClassGood : TestBaseClass { [STAThread] - public override string DoSomething(string s) + public override string DoSomething (string s) { return base.DoSomething (s); } + public override string DoSomething () + { + return base.DoSomething (":P"); + } [FileIOPermission (SecurityAction.Demand)] - public override string ToString() + public override string ToString () { return base.ToString (); } - public override bool Equals(object obj) + public override bool Equals (object obj) { if (obj == null) return false; else return base.Equals (obj); + } + } + + private class TestClassAlsoGood : ApplicationException { + public override bool Equals (object obj) + { + if (obj.GetType () != typeof (TestClassAlsoGood)) + return false; + + return base.Equals (obj); } } private class TestClassBad : TestBaseClass { - public override string ToString() + public override string ToString () { return base.ToString (); } - public override string DoSomething(string s) + public override string DoSomething (string s) { return base.DoSomething (s); } + public override string DoSomething () + { + return base.DoSomething (); + } + } + + private class TestClassAlsoBad : ApplicationException { + public override Exception GetBaseException () + { + return base.GetBaseException (); + } + } + + private class MySimpleClass { } private Mono.Cecil.TypeDefinition SimpleClassNoMethods; [SetUp] - public void SetUp() + public void SetUp () { // Classes always have a constuctor so we hack our own methodless class. - SimpleClassNoMethods = SimpleTypes.Class; + SimpleClassNoMethods = DefinitionLoader.GetTypeDefinition (); SimpleClassNoMethods.Methods.Clear (); } [Test] - public void Good() + public void Good () { AssertRuleSuccess (); + AssertRuleSuccess (); } [Test] - public void Bad() + public void Bad () { - AssertRuleFailure (2); + AssertRuleFailure (3); + AssertRuleFailure (1); } [Test] - public void DoesNotApply() + public void DoesNotApply () { AssertRuleDoesNotApply (SimpleClassNoMethods); }