Add netcore specific span based sqldate parsing #30371
Conversation
|
|
| @@ -4513,9 +4513,15 @@ internal bool TryReadSqlValue(SqlBuffer value, SqlMetaDataPriv md, int length, T | |||
|
|
|||
| private bool TryReadSqlDateTime(SqlBuffer value, byte tdsType, int length, byte scale, TdsParserStateObject stateObj) | |||
| { | |||
| #if netcoreapp | |||
| Span<byte> datetimeBuffer = length<=10 ? stackalloc byte[length] : new byte[length]; | |||
There was a problem hiding this comment.
Later on in the method there is a switch and asserts for each of the valid date types, 10 is the largest number of bytes needed by any of those cases. There is the possibility that the method can be called with an invalid type or unusual buffer length and if those happen the old heap based behaviour will be used.
There was a problem hiding this comment.
It could be but it would require changing the more general assertion here and I wanted to keep that as close to the original as possible so it can be used in other places.
| @@ -1027,6 +1027,16 @@ internal void SetToString(string value) | |||
| _isNull = false; | |||
| } | |||
|
|
|||
| #if netcoreapp | |||
There was a problem hiding this comment.
It's recommended to separate these netcoreapp specific methods to their own files such as SqlBuffer.netcoreapp.cs
You can have a look at this pr for reference:
#29697
There was a problem hiding this comment.
The method is netcore specific because that is the only configuration that includes span. If other configurations started to include span (which I would hope they will for performance reasons) then this version could be used. Really what would best is a feature specific #define.
The methods are also direct replacements not additions so putting them side by side allows editors visibility of both versions if anything needs to change.
I can make SqlBuffer partial and move the span using versions into a netcore specific file but it won't remove the #defines in the main file, those are still needed to turn off the old way and prevent the unised methods being compiled in causing bloat.
There was a problem hiding this comment.
Bearing in mind my feedback above, do you still want me to make these changes?
There was a problem hiding this comment.
@Wraith2 I'd suggest moving the new methods to netcoreapp specific files and leave the #defines inside main file be as is. This is something we discussed earlier for the PR I referenced, and the decision was to try and reduce #if netcoreapp as much as possible.
There was a problem hiding this comment.
Done, please review and feedback any further changes.
There was a problem hiding this comment.
@Wraith2 Thanks! There are some build failures on the CI. For example:
System\Data\SqlClient\SqlBuffer.netcoreapp.cs(40,13): error CS0103: The name '_isNull' does not exist in the current context
Try building using this command to build all configurations and see errors locally: build -allConfigurations
|
I don't see any related failures in the CI but locally with all configs I see it. So, how do I find which config has the error? it's pretty clear that one of them is including only the netcore partial class which is an easy tweak once I know which to do it with I found it, the fix is easy. The problem is I can't push without merging from master and that completely screws up the entire corefx build chain at the moment. |
|
I'm failing to get my branch into a sensible state. Given my changes are so small i'm going to abandon and start afresh, i'll @ those involved in the new RP. |
fixes https://github.com/dotnet/corefx/issues/28270