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.

Sign up for GitHub

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

Jump to bottom

Buffer.BlockCopy Analyzer #5242

Merged
merged 13 commits into from
Aug 13, 2021
Merged

Buffer.BlockCopy Analyzer #5242

merged 13 commits into from
Aug 13, 2021

Conversation

mahdiva
Copy link
Contributor

@mahdiva mahdiva commented Jul 14, 2021 •
edited by pgovind

This PR adds the Analyzer for the Buffer.BlockCopy method, verifying the count argument.
More info: dotnet/runtime#33765

Fixes dotnet/runtime#33765

cc @pgovind

@mahdiva mahdiva requested a review from a team as a code owner July 14, 2021 18:49
@mahdiva mahdiva changed the title Buffer block copy analyzer 1 Buffer.BlockCopy Analyzer Jul 14, 2021
@dnfadmin
Copy link

dnfadmin commented Jul 14, 2021 •
edited

CLA assistant check
All CLA requirements met.

@Youssef1313
Copy link
Member

New analyzers shouldn't target main. I'm not sure if they should target release/6.0.1xx or release/6.0.1xx-preview7.

buyaa-n
buyaa-n reviewed Jul 17, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/BufferBlockCopyLengthAnalyzer.cs Outdated
}

ILocalSymbol localArgumentDeclaration = localReferenceOperation.Local;
if (localArgumentDeclaration is null)
Copy link
Member

Choose a reason for hiding this comment

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

I would not expect it would ever be null, does it @mavasani ?

@mahdiva mahdiva changed the base branch from main to release/6.0.1xx July 20, 2021 21:02
@mahdiva
Copy link
Contributor Author

mahdiva commented Aug 9, 2021

@Youssef1313 @buyaa-n @jeffhandley I've addressed your comments. Would appreciate it if you could review them again. Thanks!

@jeffhandley
Copy link
Member

The check failures indicate that msbuild /t:pack needs to be run (via the Visual Studio Developer Command Prompt).

jeffhandley
jeffhandley approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Approved with the condition of running msbuild /t:Pack and pushing the changes from that to resolve the failing checks.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/BufferBlockCopyLengthAnalyzer.cs Show resolved Hide resolved
buyaa-n
buyaa-n approved these changes Aug 10, 2021
View reviewed changes
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.

Added one more comment, overall the PR LGTM by assuming that the #5353 will be resolved within 6.0

@codecov
Copy link

codecov bot commented Aug 10, 2021 •
edited

Codecov Report

Merging #5242 (38a5e91) into release/6.0.1xx (6e158d2) will decrease coverage by 0.00%.
The diff coverage is 93.81%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5242      +/-   ##
===================================================
- Coverage            95.62%   95.61%   -0.01%     
===================================================
  Files                 1238     1240       +2     
  Lines               284725   285372     +647     
  Branches             17083    17128      +45     
===================================================
+ Hits                272257   272872     +615     
- Misses               10157    10193      +36     
+ Partials              2311     2307       -4

@mahdiva mahdiva mentioned this pull request Aug 12, 2021
Open
@pgovind
Copy link
Contributor

pgovind commented Aug 13, 2021

CI passed. Merging. Thanks for the reviews everyone!

@pgovind pgovind merged commit 53d5560 into dotnet:release/6.0.1xx Aug 13, 2021
@jmarolf jmarolf mentioned this pull request Aug 16, 2021
Merged
This was referenced Aug 22, 2021
Closed
Closed
This was referenced Aug 30, 2021
Merged
Merged
Merged
This was referenced Sep 10, 2021
Merged
Merged
@jmarolf jmarolf mentioned this pull request Jan 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffhandley jeffhandley jeffhandley approved these changes

@pgovind pgovind pgovind left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

@mavasani mavasani Awaiting requested review from mavasani

Assignees

@mahdiva mahdiva

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass number of bytes to Buffer.BlockCopy
6 participants
@mahdiva @dnfadmin @Youssef1313 @jeffhandley @pgovind @buyaa-n