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

Provide more flexibility in the SafeHandle constructor analyzer's visibility detection #5239

Merged
merged 9 commits into from
Aug 13, 2021

Conversation

jkoritzinsky
Copy link
Member

Allow internal constructors to be used on internal SafeHandle-derived types, and allow private protected constructors to be used on private protected SafeHandle-derived types.

Fixes #5231

cc: @stephentoub

…ibility detection

Allow internal constructors to be used on internal SafeHandle-derived types, and allow private protected constructors to be used on private protected SafeHandle-derived types.

Fixes dotnet#5231

cc: @stephentoub
@jkoritzinsky jkoritzinsky requested a review from a team as a code owner July 12, 2021 23:26
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I think this PR should target main since it's a false positive fix (in case the analyzer is already in main - if it's not, then ignore the comment).

@jkoritzinsky
Copy link
Member Author

@mavasani Which branch should this PR target? release/6.0.1xx or main?

I don't think I understand the process in this repo for determining which branch to target.

@Youssef1313
Copy link
Member

@mavasani Which branch should this PR target? release/6.0.1xx or main?

I don't think I understand the process in this repo for determining which branch to target.

@jkoritzinsky Please ignore my comment, the analyzer isn't in main.

For the process question, I think it is that new analyzers are added to the next release branch (to avoid breaking changes).
False positives are in main (but obviously if an analyzer not in main, then target the release branch it's in)
No clue about false negatives, probably it depends.

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #5239 (a66a2ec) into release/6.0.1xx (92cc133) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           release/6.0.1xx    #5239   +/-   ##
================================================
  Coverage            95.60%   95.60%           
================================================
  Files                 1236     1236           
  Lines               283895   283949   +54     
  Branches             17032    17036    +4     
================================================
+ Hits                271414   271469   +55     
  Misses               10179    10179           
+ Partials              2302     2301    -1     

@RikkiGibson
Copy link
Contributor

@dotnet/roslyn-analysis for review

@jkoritzinsky
Copy link
Member Author

I do not have permission to merge, so can someone merge this for me when it has been sufficiently reviewed/approved?

@buyaa-n
Copy link
Member

buyaa-n commented Aug 13, 2021

I do not have permission to merge, so can someone merge this for me when it has been sufficiently reviewed/approved?

@jkoritzinsky is this ready to merge?

@jkoritzinsky
Copy link
Member Author

As far as I know this is ready for merge.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM merging

@buyaa-n buyaa-n merged commit 3981640 into dotnet:release/6.0.1xx Aug 13, 2021
@jmarolf jmarolf mentioned this pull request Aug 16, 2021
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

6 participants