- Notifications
You must be signed in to change notification settings - Fork311
Merge | BufferWriterExtensions#3264
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/ArrayBufferWriter.netfx.cs:12
- The PR description indicates that ArrayBufferWriter should reside within the System.Buffers namespace; please verify if updating the namespace from 'Microsoft.Data.SqlClient' to 'Microsoft.Data.SqlClient.Utilities' is intentional or if it should be aligned with the description.
namespace Microsoft.Data.SqlClient.Utilities/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just one comment about class name.
| namespace Microsoft.Data.SqlClient.Utilities | ||
| { | ||
| /// <summary> | ||
| /// This is a collection of general object pools that can be reused as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't seem general. It's a pool of ArrayBufferWriters. Should the class name be ArrayBufferWriterPools ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the theory is that the class will have more object pools added to it as we go. I'm not sure what other pools might be added, but I think that's the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh I see. This class is basically a namespace to hold static instances of a variety of ObjectPool specializations.
More of a "collection of specific ObjectPool instances that can be reused..."
codecovbot commentedApr 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3264 +/- ##==========================================- Coverage 72.81% 72.80% -0.02%========================================== Files 297 296 -1 Lines 59661 59616 -45 ==========================================- Hits 43444 43405 -39+ Misses 16217 16211 -6
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7f30536 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description:
This one is super easy, move the BufferWriterExtension class to the common project. Specifically, to a new Utilities namespace, with the .netfx suffix. While we're at it, also move the ArrayBufferWriter class to a Utilities namespace and rename it to use the .netfx suffix.Update: There is now scope creep in this PR... BufferWriterExtension class has now been moved to a new Utilities namespace in the common project. The original change moved ArrayBufferWriter to the utilities namespace, but this was not ideal. Since ArrayBufferWriter is meant to be like a polyfill for netfx (ArrayBufferWriter was not added to dotnet until netstandard2.0), it should (unfortunatly) be in the System.Buffers namespace (otherwise we will need a #if for each time it is used, to change namespace). As such, this file was moved to the System/Buffers folder in the common project. The netfx project was updated as such.
I considered merging the BufferWriterExtension into ArrayBufferWriter but 1) ArrayBufferWriter is an exact copy of the dotnet runtime implementation (since it is not part of net framework), 2) there are no tests for it, 3) we can't add tests because the type is
internal, and 4) ArrayBufferWriter is technically generic, while BufferWriterExtensions is not generic. It just makes it kind of a low roi change, so I'll just live it as-is.Scope Creep: During CI, a reference to ArrayBufferWriter was failing in the SqlObjectPools class. Since this is definitely a utility, I moved it and the SqlObjectPool classes to the utilities namespace. I then renamed them to drop the "Sql" from the class names. I also think that the thread-safe implementation of SqlObjectPools is a bit overcomplicated, so I replaced it with a much simpler
Lazyobject (which is thread-safe already).Testing: Moving the files should not have issues, rewriting SqlObjectPool to use Lazy shouldn't have impact (since it's still thread-safe).