From 17c87b15b432d3ac6f34b1785082e14cf430c6b8 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 20 Jun 2018 18:23:37 -0700 Subject: [PATCH 01/11] Use nameof in module specification argument errors --- .../engine/Modules/ModuleSpecification.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index 7e854fa9dfe..c2ae7a349d0 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -45,7 +45,7 @@ public ModuleSpecification(string moduleName) { if (string.IsNullOrEmpty(moduleName)) { - throw new ArgumentNullException("moduleName"); + throw new ArgumentNullException(nameof(moduleName)); } this.Name = moduleName; // Alias name of miniumVersion @@ -67,7 +67,7 @@ public ModuleSpecification(Hashtable moduleSpecification) { if (moduleSpecification == null) { - throw new ArgumentNullException("moduleSpecification"); + throw new ArgumentNullException(nameof(moduleSpecification)); } var exception = ModuleSpecificationInitHelper(this, moduleSpecification); @@ -166,7 +166,7 @@ internal ModuleSpecification(PSModuleInfo moduleInfo) { if (moduleInfo == null) { - throw new ArgumentNullException("moduleInfo"); + throw new ArgumentNullException(nameof(moduleInfo)); } this.Name = moduleInfo.Name; From c8fe6b274e41f13ce15f83696ab4d6c4ec6093bb Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 20 Jun 2018 18:26:42 -0700 Subject: [PATCH 02/11] Factor out field ToString() call --- .../engine/Modules/ModuleSpecification.cs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index c2ae7a349d0..3a27480be02 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -91,31 +91,35 @@ internal static Exception ModuleSpecificationInitHelper(ModuleSpecification modu { foreach (DictionaryEntry entry in hashtable) { - if (entry.Key.ToString().Equals("ModuleName", StringComparison.OrdinalIgnoreCase)) + string field = entry.Key.ToString(); + + if (field.Equals("ModuleName", StringComparison.OrdinalIgnoreCase)) { moduleSpecification.Name = LanguagePrimitives.ConvertTo(entry.Value); } - else if (entry.Key.ToString().Equals("ModuleVersion", StringComparison.OrdinalIgnoreCase)) + else if (field.Equals("ModuleVersion", StringComparison.OrdinalIgnoreCase)) { moduleSpecification.Version = LanguagePrimitives.ConvertTo(entry.Value); } - else if (entry.Key.ToString().Equals("RequiredVersion", StringComparison.OrdinalIgnoreCase)) + else if (field.Equals("RequiredVersion", StringComparison.OrdinalIgnoreCase)) { moduleSpecification.RequiredVersion = LanguagePrimitives.ConvertTo(entry.Value); } - else if (entry.Key.ToString().Equals("MaximumVersion", StringComparison.OrdinalIgnoreCase)) + else if (field.Equals("MaximumVersion", StringComparison.OrdinalIgnoreCase)) { moduleSpecification.MaximumVersion = LanguagePrimitives.ConvertTo(entry.Value); ModuleCmdletBase.GetMaximumVersion(moduleSpecification.MaximumVersion); } - else if (entry.Key.ToString().Equals("GUID", StringComparison.OrdinalIgnoreCase)) + else if (field.Equals("GUID", StringComparison.OrdinalIgnoreCase)) { moduleSpecification.Guid = LanguagePrimitives.ConvertTo(entry.Value); } else { if (badKeys.Length > 0) + { badKeys.Append(", "); + } badKeys.Append("'"); badKeys.Append(entry.Key.ToString()); badKeys.Append("'"); From 6de7b6146a55d9ea059aec505a990c50b578440f Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 20 Jun 2018 18:32:41 -0700 Subject: [PATCH 03/11] Use StringBuilder for ToString() implementation --- .../engine/Modules/ModuleSpecification.cs | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index 3a27480be02..c72f24a4432 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -186,36 +186,39 @@ internal ModuleSpecification(PSModuleInfo moduleInfo) /// public override string ToString() { - string moduleSpecString = string.Empty; if (Guid == null && Version == null && RequiredVersion == null && MaximumVersion == null) { - moduleSpecString = Name; + return Name; + } + + var moduleSpecBuilder = new StringBuilder(); + + moduleSpecBuilder.Append("@{ ModuleName = '").Append(Name).Append("'"); + + if (Guid != null) + { + moduleSpecBuilder.Append("; Guid = '{").Append(Guid).Append("}' "); + } + + if (RequiredVersion != null) + { + moduleSpecBuilder.Append("; RequiredVersion = '").Append(RequiredVersion).Append("'"); } else { - moduleSpecString = "@{ ModuleName = '" + Name + "'"; - if (Guid != null) + if (Version != null) { - moduleSpecString += "; Guid = '{" + Guid + "}' "; + moduleSpecBuilder.Append("; ModuleVersion = '").Append(Version).Append("'"); } - if (RequiredVersion != null) + if (MaximumVersion != null) { - moduleSpecString += "; RequiredVersion = '" + RequiredVersion + "'"; + moduleSpecBuilder.Append("; MaximumVersion = '").Append(MaximumVersion).Append("'"); } - else - { - if (Version != null) - { - moduleSpecString += "; ModuleVersion = '" + Version + "'"; - } - if (MaximumVersion != null) - { - moduleSpecString += "; MaximumVersion = '" + MaximumVersion + "'"; - } - } - moduleSpecString += " }"; } - return moduleSpecString; + + moduleSpecBuilder.Append(" }"); + + return moduleSpecBuilder.ToString(); } /// From 4d2d5687454b22057544201eee4934998a9693fb Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 20 Jun 2018 18:36:35 -0700 Subject: [PATCH 04/11] Simplify equality comparison logic --- .../engine/Modules/ModuleSpecification.cs | 60 +++---------------- 1 file changed, 8 insertions(+), 52 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index c72f24a4432..a7eeeeda0fc 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -277,61 +277,17 @@ internal class ModuleSpecificationComparer : IEqualityComparer Date: Wed, 20 Jun 2018 18:40:35 -0700 Subject: [PATCH 05/11] [Feature] Simplify hashcode logic --- .../engine/Modules/ModuleSpecification.cs | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index a7eeeeda0fc..bcd8d3bf032 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -273,8 +273,17 @@ public static bool TryParse(string input, out ModuleSpecification result) public Version RequiredVersion { get; internal set; } } + /// + /// Compares two ModuleSpecification objects for equality. + /// internal class ModuleSpecificationComparer : IEqualityComparer { + /// + /// Check if two module specifications are property-wise equal. + /// + /// + /// + /// True if the specifications are equal, false otherwise. public bool Equals(ModuleSpecification x, ModuleSpecification y) { if (x == y) @@ -290,31 +299,20 @@ public bool Equals(ModuleSpecification x, ModuleSpecification y) && String.Equals(x.MaximumVersion, y.MaximumVersion); } + /// + /// Get a property-based hashcode for a ModuleSpecification object. + /// + /// The module specification for the object. + /// A hashcode that is always the same for any module specification with the same properties. public int GetHashCode(ModuleSpecification obj) { int result = 0; - if (obj != null) + foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) { - if (obj.Name != null) - { - result = result ^ obj.Name.GetHashCode(); - } - if (obj.Guid.HasValue) - { - result = result ^ obj.Guid.GetHashCode(); - } - if (obj.Version != null) - { - result = result ^ obj.Version.GetHashCode(); - } - if (obj.MaximumVersion != null) - { - result = result ^ obj.MaximumVersion.GetHashCode(); - } - if (obj.RequiredVersion != null) + if (property != null) { - result = result ^ obj.RequiredVersion.GetHashCode(); + result ^= property.GetHashCode(); } } From 77d26464f3171663e6361562eaa283910f1690ac Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 21 Jun 2018 09:21:01 -0700 Subject: [PATCH 06/11] Use better hashing algorithm --- .../engine/Modules/ModuleSpecification.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index bcd8d3bf032..e1db68e4e88 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -304,15 +304,25 @@ public bool Equals(ModuleSpecification x, ModuleSpecification y) /// /// The module specification for the object. /// A hashcode that is always the same for any module specification with the same properties. + /// + /// The algorithm in this method is adapted from a StackOverflow answer from Jon Skeet: https://stackoverflow.com/a/263416/9944203 + /// public int GetHashCode(ModuleSpecification obj) { - int result = 0; + // Two ordinary prime numbers for hashing + int p = 23; + int q = 59; - foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) + int result = p; + + unchecked { - if (property != null) + foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) { - result ^= property.GetHashCode(); + if (property != null) + { + result = q * result + property.GetHashCode(); + } } } From 89370a5867c70f5dbe1b242aed1eb17715637586 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 21 Jun 2018 09:22:43 -0700 Subject: [PATCH 07/11] Add null check to GetHashCode() method --- .../engine/Modules/ModuleSpecification.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index e1db68e4e88..99dff687a12 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -309,6 +309,11 @@ public bool Equals(ModuleSpecification x, ModuleSpecification y) /// public int GetHashCode(ModuleSpecification obj) { + if (obj == null) + { + return 0; + } + // Two ordinary prime numbers for hashing int p = 23; int q = 59; From 7ef9701d99a90a91099a044450fd60d0b489d46b Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 21 Jun 2018 09:32:45 -0700 Subject: [PATCH 08/11] Improve unchecked usage --- .../engine/Modules/ModuleSpecification.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index 99dff687a12..f255018cec8 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -320,14 +320,11 @@ public int GetHashCode(ModuleSpecification obj) int result = p; - unchecked + foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) { - foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) + if (property != null) { - if (property != null) - { - result = q * result + property.GetHashCode(); - } + result = unchecked(q * result + property.GetHashCode()); } } From 8d3459792617a51e4f4e5a0ae72a77190cbcc8b3 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 21 Jun 2018 11:07:16 -0700 Subject: [PATCH 09/11] Change multiplication prime to use 31 --- .../engine/Modules/ModuleSpecification.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index f255018cec8..c8ae871ea82 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -314,9 +314,11 @@ public int GetHashCode(ModuleSpecification obj) return 0; } - // Two ordinary prime numbers for hashing + // Initial prime value int p = 23; - int q = 59; + + // Use 31 for multiplication. See https://computinglife.wordpress.com/2008/11/20/why-do-hash-functions-use-prime-numbers/ + int q = 31; int result = p; From 74f1b34d214cf8f8aa0c100d29cfde71bd5ed387 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 21 Jun 2018 21:49:42 -0700 Subject: [PATCH 10/11] Unroll loop in GetHashCode to prevent allocation --- .../engine/Modules/ModuleSpecification.cs | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index c8ae871ea82..d31845ccbce 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -322,12 +322,29 @@ public int GetHashCode(ModuleSpecification obj) int result = p; - foreach (object property in new object[] { obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion }) + if (obj.Name != null) { - if (property != null) - { - result = unchecked(q * result + property.GetHashCode()); - } + result = unchecked(q * result + obj.Name.GetHashCode()); + } + + if (obj.Guid != null) + { + result = unchecked(q * result + obj.Guid.GetHashCode()); + } + + if (obj.RequiredVersion != null) + { + result = unchecked(q * result + obj.RequiredVersion.GetHashCode()); + } + + if (obj.Version != null) + { + result = unchecked(q * result + obj.Version.GetHashCode()); + } + + if (obj.MaximumVersion != null) + { + result = unchecked(q * result + obj.MaximumVersion.GetHashCode()); } return result; From cd065f883ecf37e8255f2a990c2ebb649cda4db1 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Tue, 26 Jun 2018 10:33:03 -0700 Subject: [PATCH 11/11] Use corefx's HashCode.Combine() method --- .../engine/Modules/ModuleSpecification.cs | 38 +------------------ 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs index d31845ccbce..775b14a555e 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs @@ -304,9 +304,6 @@ public bool Equals(ModuleSpecification x, ModuleSpecification y) /// /// The module specification for the object. /// A hashcode that is always the same for any module specification with the same properties. - /// - /// The algorithm in this method is adapted from a StackOverflow answer from Jon Skeet: https://stackoverflow.com/a/263416/9944203 - /// public int GetHashCode(ModuleSpecification obj) { if (obj == null) @@ -314,40 +311,7 @@ public int GetHashCode(ModuleSpecification obj) return 0; } - // Initial prime value - int p = 23; - - // Use 31 for multiplication. See https://computinglife.wordpress.com/2008/11/20/why-do-hash-functions-use-prime-numbers/ - int q = 31; - - int result = p; - - if (obj.Name != null) - { - result = unchecked(q * result + obj.Name.GetHashCode()); - } - - if (obj.Guid != null) - { - result = unchecked(q * result + obj.Guid.GetHashCode()); - } - - if (obj.RequiredVersion != null) - { - result = unchecked(q * result + obj.RequiredVersion.GetHashCode()); - } - - if (obj.Version != null) - { - result = unchecked(q * result + obj.Version.GetHashCode()); - } - - if (obj.MaximumVersion != null) - { - result = unchecked(q * result + obj.MaximumVersion.GetHashCode()); - } - - return result; + return HashCode.Combine(obj.Name, obj.Guid, obj.RequiredVersion, obj.Version, obj.MaximumVersion); } }