Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Removing redundant comparison with "null" - #53

Merged
dotnet-bot merged 1 commit into
dotnet:masterfrom
AndreyAkinshin:master
Nov 13, 2014
Merged

dotnet-bot merged 1 commit into
dotnet:masterfrom
AndreyAkinshin:master

Conversation

@AndreyAkinshin

Copy link
Copy Markdown
Member

Removing redundant comparison with "null" ((!(obj is Vector)) is superset condition for (obj == null))

@mellinoe

Copy link
Copy Markdown
Contributor

Looks reasonable to me.

@mikaeldui

Copy link
Copy Markdown

Isn't "obj == null" a lot less expensive than "is"?

@AndreyAkinshin

Copy link
Copy Markdown
Member Author

@mikaeldui, yes, but in the case of a non-null obj with wrong type, the whole condition will more expensive.
I think, we should remove the redundant check.

@joshfree

Copy link
Copy Markdown
Member

👍 LGTM

@stephentoub

Copy link
Copy Markdown
Member

I ran some microbenchmarks, which were either inconclusive or slightly favored the change. Given that, and that this change makes the code cleaner, LGTM2.

@robinwassen

Copy link
Copy Markdown

Feels like quite a minor improvement - but Andreys solution is more favorable than the existing since it does not try to outsmart the compiler.

There is also a third style that could be used that looks like the following:

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public override bool Equals(object obj)
    {
        Vector2 v = obj as Vector2;

        if (!v)
            return false;

        return Equals(v);
    }

Hard to tell which one is nicest, but guess it does not matter that much since this issue probably won't have any real impact on the readability or performance.

@dotnet-bot
dotnet-bot merged commit b8c99c0 into dotnet:master Nov 13, 2014
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
luhenry pushed a commit to luhenry/corefx that referenced this pull request May 24, 2018
Mono specific conditionals in preparation of getting `System.Buffers.Text`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants