Skip to content

Conversation

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented Oct 12, 2019

PR Summary

Refactor IsNullLike() to evaluate the most commonly-expected null-like values first.

New order of evaluation:

  1. IsNull(value):
    1. value == null
    2. value == AutomationNull.Value
  2. value == DBNull.Value
  3. value == AutomationNull.Value

PR Context

IsNullLike() is used in binders like -eq (e.g., $null -eq $value) and it makes the most sense to evaluate in order of the most common null/null-like values first so that the code path short-circuits in the most common situations.

PR Checklist

@vexx32 vexx32 requested review from daxian-dbw and iSazonov October 12, 2019 19:32
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 12, 2019

It was in #9794 (milestone 7.0-Preview2) as a breaking change without PowerShell Committee approve. So add the labels.

@iSazonov iSazonov added Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 12, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Oct 12, 2019
@iSazonov iSazonov requested a review from SteveL-MSFT October 12, 2019 19:46
@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 13, 2019

@iSazonov the committee approved the change in the original issue (see #9561 (comment)) 🙂

@iSazonov
Copy link
Collaborator

My concern about order.

@daxian-dbw
Copy link
Member

Why is changing the order a breaking change? It doesn't change the behavior of this method, does it?

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 15, 2019

Nope, it still checks the same things, just in a different order. 🙂

@iSazonov iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 15, 2019
@iSazonov
Copy link
Collaborator

I see the logic is more simple than I thought.

@iSazonov iSazonov self-assigned this Oct 16, 2019
@iSazonov iSazonov merged commit 25286eb into PowerShell:master Oct 16, 2019
@iSazonov iSazonov changed the title LanguagePrimitives.IsNullLike() - use more effective evaluation order Use more effective evaluation order in LanguagePrimitives.IsNullLike() Oct 16, 2019
@vexx32 vexx32 deleted the IsNullLike branch October 16, 2019 10:11
@ghost
Copy link

ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants