-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eval cond perf #6859
Eval cond perf #6859
Changes from all commits
e02b3d9
64354ff
762ac1d
f9456c4
16a30b3
6e4c0d4
2aac6e2
0134385
625fd3f
62760c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||||||
// Copyright (c) Microsoft. All rights reserved. | ||||||||||||||||||||||||||||||||||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
using Microsoft.Build.Shared; | ||||||||||||||||||||||||||||||||||
using System.Diagnostics; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
namespace Microsoft.Build.Evaluation | ||||||||||||||||||||||||||||||||||
|
@@ -17,12 +18,14 @@ internal sealed class NotExpressionNode : OperatorExpressionNode | |||||||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||||||||
internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) | ||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||
return !LeftChild.BoolEvaluate(state); | ||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
internal override bool CanBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) | ||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||
return LeftChild.CanBoolEvaluate(state); | ||||||||||||||||||||||||||||||||||
ProjectErrorUtilities.VerifyThrowInvalidProject | ||||||||||||||||||||||||||||||||||
(LeftChild.TryBoolEvaluate(state, out bool boolValue), | ||||||||||||||||||||||||||||||||||
state.ElementLocation, | ||||||||||||||||||||||||||||||||||
"ExpressionDoesNotEvaluateToBoolean", | ||||||||||||||||||||||||||||||||||
LeftChild.GetUnexpandedValue(state), | ||||||||||||||||||||||||||||||||||
LeftChild.GetExpandedValue(state), | ||||||||||||||||||||||||||||||||||
state.Condition); | ||||||||||||||||||||||||||||||||||
Comment on lines
+21
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
to avoid the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just finished pushing the VerifyThrow --> Throw change in the other PR. |
||||||||||||||||||||||||||||||||||
return !boolValue; | ||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,46 +38,29 @@ internal abstract class NumericComparisonExpressionNode : OperatorExpressionNode | |
/// </summary> | ||
internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) | ||
{ | ||
bool isLeftNum = LeftChild.CanNumericEvaluate(state); | ||
bool isLeftVersion = LeftChild.CanVersionEvaluate(state); | ||
bool isRightNum = RightChild.CanNumericEvaluate(state); | ||
bool isRightVersion = RightChild.CanVersionEvaluate(state); | ||
bool isNumeric = isLeftNum && isRightNum; | ||
bool isVersion = isLeftVersion && isRightVersion; | ||
bool isValidComparison = isNumeric || isVersion || (isLeftNum && isRightVersion) || (isLeftVersion && isRightNum); | ||
bool isLeftNum = LeftChild.TryNumericEvaluate(state, out double leftNum); | ||
bool isLeftVersion = LeftChild.TryVersionEvaluate(state, out Version leftVersion); | ||
bool isRightNum = RightChild.TryNumericEvaluate(state, out double rightNum); | ||
bool isRightVersion = RightChild.TryVersionEvaluate(state, out Version rightVersion); | ||
|
||
ProjectErrorUtilities.VerifyThrowInvalidProject | ||
(isValidComparison, | ||
((isLeftNum || isLeftVersion) && (isRightNum || isRightVersion), | ||
state.ElementLocation, | ||
"ComparisonOnNonNumericExpression", | ||
state.Condition, | ||
/* helpfully display unexpanded token and expanded result in error message */ | ||
LeftChild.CanNumericEvaluate(state) ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), | ||
LeftChild.CanNumericEvaluate(state) ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); | ||
isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), | ||
isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also probably worth optimizing by not making these calls in the happy case. |
||
|
||
// If the values identify as numeric, make that comparison instead of the Version comparison since numeric has a stricter definition | ||
if (isNumeric) | ||
return (isLeftNum, isLeftVersion, isRightNum, isRightVersion) switch | ||
{ | ||
return Compare(LeftChild.NumericEvaluate(state), RightChild.NumericEvaluate(state)); | ||
} | ||
else if (isVersion) | ||
{ | ||
return Compare(LeftChild.VersionEvaluate(state), RightChild.VersionEvaluate(state)); | ||
} | ||
|
||
// If the numbers are of a mixed type, call that specific Compare method | ||
if (isLeftNum && isRightVersion) | ||
{ | ||
return Compare(LeftChild.NumericEvaluate(state), RightChild.VersionEvaluate(state)); | ||
} | ||
else if (isLeftVersion && isRightNum) | ||
{ | ||
return Compare(LeftChild.VersionEvaluate(state), RightChild.NumericEvaluate(state)); | ||
} | ||
(true, _, true, _) => Compare(leftNum, rightNum), | ||
(_, true, _, true) => Compare(leftVersion, rightVersion), | ||
(true, _, _, true) => Compare(leftNum, rightVersion), | ||
(_, true, true, _) => Compare(leftVersion, rightNum), | ||
|
||
// Throw error here as this code should be unreachable | ||
ErrorUtilities.ThrowInternalErrorUnreachable(); | ||
return false; | ||
_ => false | ||
}; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,29 +19,29 @@ internal sealed class OrExpressionNode : OperatorExpressionNode | |
internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) | ||
{ | ||
ProjectErrorUtilities.VerifyThrowInvalidProject | ||
(LeftChild.CanBoolEvaluate(state), | ||
(LeftChild.TryBoolEvaluate(state, out bool leftBool), | ||
state.ElementLocation, | ||
"ExpressionDoesNotEvaluateToBoolean", | ||
LeftChild.GetUnexpandedValue(state), | ||
LeftChild.GetExpandedValue(state), | ||
Comment on lines
25
to
26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These calls can be avoided in the happy case. |
||
state.Condition); | ||
|
||
if (LeftChild.BoolEvaluate(state)) | ||
if (leftBool) | ||
{ | ||
// Short circuit | ||
return true; | ||
} | ||
else | ||
{ | ||
ProjectErrorUtilities.VerifyThrowInvalidProject | ||
(RightChild.CanBoolEvaluate(state), | ||
(RightChild.TryBoolEvaluate(state, out bool rightBool), | ||
state.ElementLocation, | ||
"ExpressionDoesNotEvaluateToBoolean", | ||
RightChild.GetUnexpandedValue(state), | ||
RightChild.GetExpandedValue(state), | ||
Comment on lines
40
to
41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These calls can be avoided in the happy case. |
||
state.Condition); | ||
|
||
return RightChild.BoolEvaluate(state); | ||
return rightBool; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comment 👍🏻