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

CA1810: Report only if field belongs to ctor's type #5812

Closed
wants to merge 1 commit into from

Conversation

Evangelink
Copy link
Member

Fix #5784

@Evangelink Evangelink requested a review from a team as a code owner January 24, 2022 17:25
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #5812 (56a1030) into main (f471d33) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5812      +/-   ##
==========================================
- Coverage   95.58%   95.57%   -0.01%     
==========================================
  Files        1284     1284              
  Lines      296834   296866      +32     
  Branches    18101    18103       +2     
==========================================
+ Hits       283725   283738      +13     
- Misses      10670    10680      +10     
- Partials     2439     2448       +9     

{
static C2()
{
C1.Size = 42;
Copy link
Contributor

@mavasani mavasani Jan 25, 2022

Choose a reason for hiding this comment

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

What would be the result if the static ctor initializes both static fields of another type and also static fields of the containing type? Would it match our expected result in that case? Can you add a unit test?

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the implementation, but need to verify this is truly a false positive. CA1810 is a performance analyzer that I don't believe is enabled by default, so we'll want to make sure the analyzer is fully preserving the original intent after this change. Further discussion will go in #5784.

@Evangelink
Copy link
Member Author

@sharwell Please let me know if you think that's better to close PR.

@sharwell
Copy link
Member

@Evangelink yep, the analyzer is behaving by design here

@sharwell sharwell closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

false-positive for CA1810
4 participants