Merge | BufferWriterExtensions #3264
Conversation
There was a problem hiding this comment.
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). |
| 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.
This doesn't seem general. It's a pool of ArrayBufferWriters. Should the class name be ArrayBufferWriterPools ?
There was a problem hiding this comment.
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.
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..."
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:
|
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).