Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -79,31 +79,30 @@ public override bool Equals(Object inputObject)
{
ObjectCommandPropertyValue objectCommandPropertyValueObject = inputObject as ObjectCommandPropertyValue;
if (objectCommandPropertyValueObject == null)
{
return false;
}

object baseObject = PSObject.Base(PropertyValue);
object inComingbaseObjectPropertyValue = PSObject.Base(objectCommandPropertyValueObject.PropertyValue);

IComparable baseObjectComparable = baseObject as IComparable;
if (baseObject is IComparable)
{
var success = LanguagePrimitives.TryCompare(baseObject, inComingbaseObjectPropertyValue, CaseSensitive, Culture, out int result);
return success && result == 0;
}

if (baseObjectComparable != null)
if (baseObject == null && inComingbaseObjectPropertyValue == null)
{
return (LanguagePrimitives.Compare(baseObject, inComingbaseObjectPropertyValue, CaseSensitive, Culture) == 0);
return true;
}
else
if (baseObject != null && inComingbaseObjectPropertyValue != null)
{
if (baseObject == null && inComingbaseObjectPropertyValue == null)
{
return true;
}
if (baseObject != null && inComingbaseObjectPropertyValue != null)
{
return baseObject.ToString().Equals(inComingbaseObjectPropertyValue.ToString(), StringComparison.OrdinalIgnoreCase);
}

// One of the property values being compared is null.
return false;
return baseObject.ToString().Equals(inComingbaseObjectPropertyValue.ToString(), StringComparison.OrdinalIgnoreCase);
}

// One of the property values being compared is null.
return false;
}

/// <summary>
Expand Down Expand Up @@ -190,35 +189,30 @@ public int Compare(object first, object second)
{
// This method will never throw exceptions, two null
// objects are considered the same
if (IsValueNull(first) && IsValueNull(second)) return 0;
if (IsValueNull(first) && IsValueNull(second))
{
return 0;
}

PSObject firstMsh = first as PSObject;
if (firstMsh != null)
if (first is PSObject firstMsh)
{
first = firstMsh.BaseObject;
}

PSObject secondMsh = second as PSObject;
if (secondMsh != null)
if (second is PSObject secondMsh)
{
second = secondMsh.BaseObject;
}

try
{
return LanguagePrimitives.Compare(first, second, !_caseSensitive, _cultureInfo) * (_ascendingOrder ? 1 : -1);
}
catch (InvalidCastException)
{
}
catch (ArgumentException)
if (LanguagePrimitives.TryCompare(first, second, !_caseSensitive, _cultureInfo, out int result))
{
// Note that this will occur if the objects do not support
// IComparable. We fall back to comparing as strings.
return result * (_ascendingOrder ? 1 : -1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get some win if we exclude multiplication?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave that to the compiler and the JIT. They usually do a good job on things like this.

}

// Note that this will occur if the objects do not support
// IComparable. We fall back to comparing as strings.

// being here means the first object doesn't support ICompare
// or an Exception was raised win Compare
string firstString = PSObject.AsPSObject(first).ToString();
string secondString = PSObject.AsPSObject(second).ToString();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,19 +83,14 @@ internal bool UpdateGroupingKeyValue(PSObject so)

private static bool IsEqual(object first, object second)
{
try
if (LanguagePrimitives.TryCompare(first, second, true, CultureInfo.CurrentCulture, out int result))
{
return LanguagePrimitives.Compare(first, second, true, CultureInfo.CurrentCulture) == 0;
}
catch (InvalidCastException)
{
}
catch (ArgumentException)
{
// Note that this will occur if the objects do not support
// IComparable. We fall back to comparing as strings.
return result == 0;
}

// Note that this will occur if the objects do not support
// IComparable. We fall back to comparing as strings.

// being here means the first object doesn't support ICompare
// or an Exception was raised win Compare
string firstString = PSObject.AsPSObject(first).ToString();
Expand Down
180 changes: 147 additions & 33 deletions src/System.Management.Automation/engine/LanguagePrimitives.cs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,30 @@ public static bool Equals(object first, object second, bool ignoreCase, IFormatP
return false;
}

/// <summary>
/// Helper method for [Try]Compare to determine object ordering with null.
/// </summary>
/// <param name="value">the numeric value to compare to null</param>
/// <param name="numberIsRightHandSide">True if the number to compare is on the right hand side if the comparison.</param>
private static int CompareObjectToNull(object value, bool numberIsRightHandSide)
{
var i = numberIsRightHandSide ? -1 : 1;

// If it's a positive number, including 0, it's greater than null
// for everything else it's less than zero...
switch (value)
{
case Int16 i16: return Math.Sign(i16) < 0 ? -i : i;
case Int32 i32: return Math.Sign(i32) < 0 ? -i : i;
case Int64 i64: return Math.Sign(i64) < 0 ? -i : i;
case sbyte sby: return Math.Sign(sby) < 0 ? -i : i;
case float f: return Math.Sign(f) < 0 ? -i : i;
case double d: return Math.Sign(d) < 0 ? -i : i;
case decimal de: return Math.Sign(de) < 0 ? -i : i;
default: return i;
}
}

/// <summary>
/// Compare first and second, converting second to the
/// type of the first, if necessary.
Expand Down Expand Up @@ -762,43 +786,12 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP

if (first == null)
{
if (second == null)
{
return 0;
}
else
{
// If it's a positive number, including 0, it's greater than null
// for everything else it's less than zero...
switch (LanguagePrimitives.GetTypeCode(second.GetType()))
{
case TypeCode.Int16: return System.Math.Sign((Int16)second) < 0 ? 1 : -1;
case TypeCode.Int32: return System.Math.Sign((Int32)second) < 0 ? 1 : -1;
case TypeCode.Int64: return System.Math.Sign((Int64)second) < 0 ? 1 : -1;
case TypeCode.SByte: return System.Math.Sign((sbyte)second) < 0 ? 1 : -1;
case TypeCode.Single: return System.Math.Sign((System.Single)second) < 0 ? 1 : -1;
case TypeCode.Double: return System.Math.Sign((System.Double)second) < 0 ? 1 : -1;
case TypeCode.Decimal: return System.Math.Sign((System.Decimal)second) < 0 ? 1 : -1;
default: return -1;
}
}
return second == null ? 0 : CompareObjectToNull(second, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: CompareObjectToNull(second, numberIsRightHandSide:true)

}

if (second == null)
{
// If it's a positive number, including 0, it's greater than null
// for everything else it's less than zero...
switch (LanguagePrimitives.GetTypeCode(first.GetType()))
{
case TypeCode.Int16: return System.Math.Sign((Int16)first) < 0 ? -1 : 1;
case TypeCode.Int32: return System.Math.Sign((Int32)first) < 0 ? -1 : 1;
case TypeCode.Int64: return System.Math.Sign((Int64)first) < 0 ? -1 : 1;
case TypeCode.SByte: return System.Math.Sign((sbyte)first) < 0 ? -1 : 1;
case TypeCode.Single: return System.Math.Sign((System.Single)first) < 0 ? -1 : 1;
case TypeCode.Double: return System.Math.Sign((System.Double)first) < 0 ? -1 : 1;
case TypeCode.Decimal: return System.Math.Sign((System.Decimal)first) < 0 ? -1 : 1;
default: return 1;
}
return CompareObjectToNull(first, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: CompareObjectToNull(first, numberIsRightHandSide:false)

}

if (first is string firstString)
Expand Down Expand Up @@ -854,6 +847,127 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP
throw PSTraceSource.NewArgumentException("first", ExtendedTypeSystem.NotIcomparable, first.ToString());
}

/// <summary>
/// Tries to compare first and second, converting second to the type of the first, if necessary.
/// If a conversion is needed but fails, false is return.
/// </summary>
/// <param name="first">First comparison value.</param>
/// <param name="second">Second comparison value.</param>
/// <param name="result">Less than zero if first is smaller than second, more than
/// zero if it is greater or zero if they are the same.</param>
/// <returns>True if the comparison was successful, false otherwise.</returns>
public static bool TryCompare(object first, object second, out int result)
{
return TryCompare(first, second, ignoreCase: false, CultureInfo.InvariantCulture, out result);
}

/// <summary>
/// Tries to compare first and second, converting second to the type of the first, if necessary.
/// If a conversion is needed but fails, false is return.
/// </summary>
/// <param name="first">First comparison value.</param>
/// <param name="second">Second comparison value.</param>
/// <param name="ignoreCase">Used if both values are strings.</param>
/// <param name="result">Less than zero if first is smaller than second, more than zero if it is greater or zero if they are the same.</param>
/// <returns>True if the comparison was successful, false otherwise.</returns>
public static bool TryCompare(object first, object second, bool ignoreCase, out int result)
{
return TryCompare(first, second, ignoreCase, CultureInfo.InvariantCulture, out result);
}

/// <summary>
/// Tries to compare first and second, converting second to the type of the first, if necessary.
/// If a conversion is needed but fails, false is return.
/// </summary>
/// <param name="first">First comparison value.</param>
/// <param name="second">Second comparison value.</param>
/// <param name="ignoreCase">Used if both values are strings.</param>
/// <param name="formatProvider">Used in type conversions and if both values are strings.</param>
/// <param name="result">Less than zero if first is smaller than second, more than zero if it is greater or zero if they are the same.</param>
/// <returns>True if the comparison was successful, false otherwise.</returns>
/// <exception cref="ArgumentException">The parameter <paramref name="formatProvider"/> is not a <see cref="CultureInfo"/>.</exception>
public static bool TryCompare(object first, object second, bool ignoreCase, IFormatProvider formatProvider, out int result)
{
result = 0;
if (formatProvider == null)
{
formatProvider = CultureInfo.InvariantCulture;
}

if (!(formatProvider is CultureInfo culture))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we use IFormatProvider parameter type if we want get CultureInfo?

Copy link
Collaborator Author

@powercode powercode Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not inventing a new API here, just providing a Try alternative to the existing one.

If that API should be changed seems like a separate discussion.

{
throw PSTraceSource.NewArgumentException("formatProvider");
}

first = PSObject.Base(first);
second = PSObject.Base(second);

if (first == null && second == null)
{
result = 0;
return true;
}

if (first == null)
{
result = CompareObjectToNull(second, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: CompareObjectToNull(second, numberIsRighthandSide:true)

return true;
}

if (second == null)
{
// If it's a positive number, including 0, it's greater than null
// for everything else it's less than zero...
result = CompareObjectToNull(first, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Same comment

return true;
}

if (first is string firstString)
{
if (!(second is string secondString))
{
if (!TryConvertTo(second, culture, out secondString))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could replace the two if-s with one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryConvertTo() uses ConversionData.Invoke() which throws exceptions for invalid casts. Have you considered adding a "TryInvoke" method to avoid the exception, for performance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaulHigin I recently changed TryConvertTo to check the result of FigureConvertion and exit early, without invoking ConversionData.Invoke() if the ConvertionRank is None.

So, yes, I have avoided the exception in many but not all cases. Interesting take to put it on ConversionData. That is arguably where it belongs.

{
return false;
}
}

result = culture.CompareInfo.Compare(firstString, secondString, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None);
return true;
}

Type firstType = first.GetType();
Type secondType = second.GetType();
int firstIndex = TypeTableIndex(firstType);
int secondIndex = TypeTableIndex(secondType);
if (firstIndex != -1 && secondIndex != -1)
{
result = NumericCompare(first, second, firstIndex, secondIndex);
return true;
}

if (!TryConvertTo(second, firstType, culture, out object secondConverted))
{
return false;
}

if (first is IComparable firstComparable)
{
result = firstComparable.CompareTo(secondConverted);
return true;
}

if (first.Equals(second))
{
result = 0;
return true;
}

// At this point, we know that they aren't equal but we have no way of
// knowing which should compare greater than the other so we return false.
return false;
}

/// <summary>
/// Returns true if the language considers obj to be true
/// </summary>
Expand Down
58 changes: 57 additions & 1 deletion test/powershell/engine/Api/LanguagePrimitive.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Describe "Language Primitive Tests" -Tags "CI" {
}

It "Casting recursive array to bool should not cause crash" {
$a[0] = $a = [PSObject](,1)
$a[0] = $a = [PSObject](, 1)
[System.Management.Automation.LanguagePrimitives]::IsTrue($a) | Should -BeTrue
}

Expand All @@ -46,4 +46,60 @@ Describe "Language Primitive Tests" -Tags "CI" {
[System.Management.Automation.LanguagePrimitives]::GetEnumerable($dt) | ForEach-Object { $count++ }
$count | Should -Be 2
}

It "TryCompare should succeed on int and string" {
$result = $null
[System.Management.Automation.LanguagePrimitives]::TryCompare(1, "1", [ref] $result) | Should -BeTrue
$result | Should -Be 0
}

It "TryCompare should fail on int and datetime" {
$result = $null
[System.Management.Automation.LanguagePrimitives]::TryCompare(1, [datetime]::Now, [ref] $result) | Should -BeFalse
}

It "TryCompare should succeed on int and int and compare correctly smaller" {
$result = $null
[System.Management.Automation.LanguagePrimitives]::TryCompare(1, 2, [ref] $result) | Should -BeTrue
$result | Should -BeExactly -1
}

It "TryCompare should succeed on string and string and compare correctly greater" {
$result = $null
[System.Management.Automation.LanguagePrimitives]::TryCompare("bbb", "aaa", [ref] $result) | Should -BeTrue
$result | Should -BeExactly 1
}

It "TryCompare should succeed on string and string and compare case insensitive correctly" {
$result = $null
[System.Management.Automation.LanguagePrimitives]::TryCompare("AAA", "aaa", $true, [ref] $result) | Should -BeTrue
$result | Should -BeExactly 0
}

It "TryCompare with cultureInfo is culture sensitive" {
$result = $null
$swedish = [cultureinfo] 'sv-SE'
# in Swedish, åäö appears at the end of the alphabet, and should compare greater than o
$val = [System.Management.Automation.LanguagePrimitives]::TryCompare("ooo", "ååå", $false, $swedish, [ref] $result)
$val | Should -BeTrue
$result | Should -BeExactly -1
}

It "TryCompare compares greater than null as Compare" {
$result = $null

$compareResult = [System.Management.Automation.LanguagePrimitives]::Compare($null, 10)
$val = [System.Management.Automation.LanguagePrimitives]::TryCompare($null, 10, [ref] $result)
$val | Should -BeTrue
$result | Should -BeExactly $compareResult
}

It "TryCompare compares less than null as Compare" {
$result = $null

$compareResult = [System.Management.Automation.LanguagePrimitives]::Compare(10, $null)
$val = [System.Management.Automation.LanguagePrimitives]::TryCompare(10, $null, [ref] $result)
$val | Should -BeTrue
$result | Should -BeExactly $compareResult
}
}