NotificationsYou must be signed in to change notification settings
Fork311
Star927

Merge | Port #528 and #2091 to netfx#3376

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

benrr101 merged 2 commits intodotnet:mainfromedwardneal:port/pr-2091

May 29, 2025

Merged

Merge | Port #528 and #2091 to netfx#3376

benrr101 merged 2 commits intodotnet:mainfromedwardneal:port/pr-2091

May 29, 2025

Conversation

Copy link

Contributor

edwardneal commentedMay 26, 2025

Description

This ports an earlier set of memory usage reductions from netcore to netfx.

Issues

Relates to#1261.
Ports#528 and#2091.

Testing

Unit tests pass locally. I don't see anything specifically targeting these changes, so I assume that they're covered by the overall test suite - they're fairly frequently exercised.

Portdotnet#528anddotnet#2091to netfx

8b6c33a

edwardneal requested a review froma team as acode owner

May 26, 2025 15:13

Copy link

Member

cheenamalhotra commentedMay 26, 2025

/azp run

Copy link

azure-pipelinesbot commentedMay 26, 2025

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski requested changes

May 27, 2025

View reviewed changes

Copy link

Contributor

paulmedynski left a comment

Choose a reason for hiding this comment

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

A couple of spurious ? checks.

I didn't dig into the full logic that was added/modified - I don't have enough experience with SqlCommand for that yet.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs OutdatedShow resolvedHide resolved

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs OutdatedShow resolvedHide resolved

Following review - redundant null check

d60c800

Copy link

Member

cheenamalhotra commentedMay 27, 2025

/azp run

cheenamalhotra requested a review froma team

May 27, 2025 17:25

cheenamalhotra added this to the6.1-preview2 milestone

May 27, 2025

Copy link

azure-pipelinesbot commentedMay 27, 2025

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecovbot commentedMay 27, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is96.64430% with5 lines in your changes missing coverage. Please review.

Project coverage is 58.80%. Comparing base(b8948f2) to head(d60c800).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs	97.27%	4 Missing⚠️
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs	50.00%	1 Missing⚠️

❗ There is a different number of reports uploaded between BASE (b8948f2) and HEAD (d60c800). Click for more details.
HEAD has 1 upload less than BASE
Flag BASE (b8948f2) HEAD (d60c800)
addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3376      +/-   ##==========================================- Coverage   67.04%   58.80%   -8.25%==========================================  Files         300      292       -8       Lines       65376    65307      -69     ==========================================- Hits        43831    38403    -5428- Misses      21545    26904    +5359