This repository was archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Faster null checks #21736
Closed
Closed
Faster null checks #21736
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d3ae0c3
Faster null checks
benaadams ddb03ff
Feedback
benaadams 23693ef
isnot null
benaadams d85c9c5
Some more
benaadams 0ed3baf
object casts
benaadams d9ef0ec
Go all in
benaadams e4389b0
x is object => !(x is null)
benaadams 5bcb6a5
search replace error
benaadams File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ public static bool Exists(string path) | |
| { | ||
| try | ||
| { | ||
| if (path == null) | ||
| if (path is null) | ||
|
Member
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. Is here actually a difference as Roslyn generates the same code for both? BTW: here I would use |
||
| return false; | ||
| if (path.Length == 0) | ||
| return false; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is here actually a difference as Roslyn generates the same code for both?
Though I find this version more readable / nicer 😉.
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.
Can either pick consistency or ease of changing 4k+ call sites 😄
Early commits I was only picking a specific set of classes; doing it for the all
== nulland!= nullchecks is either much slower or more broad brush (or would need time to write a tool)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.
That's right 😉
!(obj is null)vs.obj != nullthe latter is more readable.obj is nullvsobj == nullI find both very readable.So it's hard to find a good balance between consistency and readability, as for
MethodInfoand the like theis-variant has to be taken perf-wise -- or adapt the compiler(s) to emit efficient code for thenull-comparison.Uh oh!
There was an error while loading. Please reload this page.
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.
Was a discussion (on twitter) about a less fugly
is not nullcheck for C#; most popular variants seemed to bex !is nullandx is !nullDon't know if there is a formal C# proposal though
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.
VB's leading the way here by already having
IsNot Nothing😉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.
Too many channels to follow 😃 Thanks for the link!
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.
It's there: dotnet/csharplang#27. Someone suggested
aintoperator as well. 😄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.
I think this is the current proposal that would end up landing the 'not' pattern: dotnet/csharplang#1350
Uh oh!
There was an error while loading. Please reload this page.
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.
For checking
not nullspecifically, there's also the syntax that falls out from Property Patterns:if (foo is {}), though I will concede that it's not quite as readable asif (foo is not null), but the outcome is the exact same.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.
if (foo is object)also works, but undid that in e4389b0 as it wasn't very clear