Skip to content
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

Avoid boxing while unpacking flags #8882

Merged
merged 1 commit into from Mar 7, 2021

Conversation

drewnoakes
Copy link
Member

Proposed changes

Eliminate boxing when in GitItemStatus bool property getters.

Enum.HasFlag boxes its argument. Replace it with a method that does not.

Test methodology

  • Manual testing

✒️ I contribute this code under The Developer Certificate of Origin.

Enum.HasFlag boxes its argument. Replace it with a method that does not.
@ghost ghost assigned drewnoakes Feb 24, 2021
@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #8882 (e457516) into master (b6905f1) will decrease coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8882      +/-   ##
==========================================
- Coverage   56.03%   55.78%   -0.25%     
==========================================
  Files         922      916       -6     
  Lines       65944    65447     -497     
  Branches    12070    11988      -82     
==========================================
- Hits        36951    36509     -442     
- Misses      25985    25986       +1     
+ Partials     3008     2952      -56     
Flag Coverage Δ
production 43.24% <100.00%> (-0.02%) ⬇️
tests 95.04% <ø> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Comment on lines +166 to +170
private bool HasFlag(Flags flags)
{
// NOTE Enum.HasFlag boxes its argument
return (flags & _flags) == flags;
}
Copy link
Member

Choose a reason for hiding this comment

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

BTW: The name of the enum Flags is misleading. It should be in singular.
I can read this better:

Suggested change
private bool HasFlag(Flags flags)
{
// NOTE Enum.HasFlag boxes its argument
return (flags & _flags) == flags;
}
private bool HasFlag(Flags flag)
{
// NOTE Enum.HasFlag boxes its argument
return (_flags & flag) == flag;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic works equally well with multiple flags. The type is a flags enum, and has a plural name.

Copy link
Member

Choose a reason for hiding this comment

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

The logic works equally well with multiple flags.

This was clear to me. But Enum.HasFlag uses only singular. And nothing else is used here.

Better readability (as in other recent PRs):

Suggested change
private bool HasFlag(Flags flags)
{
// NOTE Enum.HasFlag boxes its argument
return (flags & _flags) == flags;
}
private bool HasFlag(Flags flags)
{
// NOTE Enum.HasFlag boxes its argument
return (_flags & flags) == flags;
}

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

+1
Agree with mstv

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

IIRC .NET Core has resolved these allocations, hasn't it? #8522 makes this change obsolete.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 25, 2021
@drewnoakes
Copy link
Member Author

@RussKie what change are you looking for here?

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 6, 2021
@gerhardol
Copy link
Member

@RussKie what change are you looking for here?

I believe it was a comment that the change is not necessary as .NET5 should be coming soon
Suggest this is merged now, an improvement regardless

@RussKie RussKie merged commit 19fe539 into gitextensions:master Mar 7, 2021
@ghost ghost added this to the 3.6 milestone Mar 7, 2021
@drewnoakes drewnoakes deleted the avoid-boxing branch March 8, 2021 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants