Clean: Modernize style in ValueUtilsSmi #1351
Conversation
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs
Outdated
Show resolved
Hide resolved
Wraith2
force-pushed
the
clean-valueutils
branch
2 times, most recently
from
d2b8e8e
to
09cf9e8
Compare
|
I've cleaned up the majority of the #ifdefs by using a conditional alias to set SmiContext to object in netcore builds. The context is almost never used even in netcore and anywhere that actually needs it to be the real type wouldn't compile with the object alias. This unifies the majority of the file, see what you think @Kaur-Parminder |
cheenamalhotra
added
the
➕ Code Health
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs
Show resolved
Hide resolved
@Wraith2 changes look good to me. |
Wraith2
force-pushed
the
clean-valueutils
branch
from
60ab271
to
c59a736
Compare
Wraith2
force-pushed
the
clean-valueutils
branch
from
c59a736
to
0be5754
Compare
|
Merge conflicts resolved. |
|
|
||
| // Central checkpoint for closed recordset. | ||
| // Any code that requires an open recordset should call this method first! | ||
| // Especially any code that accesses unmanaged memory structures whose lifetime | ||
| // matches the lifetime of the unmanaged recordset. | ||
| internal void ThrowIfClosed(string operationName) | ||
| internal void ThrowIfClosed([CallerMemberName] string operationName = null) |
There was a problem hiding this comment.
Although operationName is not going to be null there might be an exception thrown for assigning null value as a default value. shouldn't we use #nullable enable and disable here and change it to string? operationName =null or as MS documents shows assign an empty string to operationName?
There was a problem hiding this comment.
We aren't using nullable contexts anywhere in this library yet. When we do then it'll be picked up as we have to go over the library to annotate it. It doesn't seem helpful until then.
| typeCode == ExtendedClrTypeCode.DBNull || | ||
| typeCode == ExtendedClrTypeCode.Empty || | ||
| CanAccessSetterDirectly(metaData, typeCode) || | ||
| value is DataFeed |
There was a problem hiding this comment.
NIT: can we put the comment back as well, I have found those comments very helpful while debugging an issue.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Central checkpoint to ensure the requested column can be accessed. | ||
| // Calling this function serves to notify that it has been accessed by the user. | ||
| [SuppressMessage("Microsoft.Performance", "CA1801:AvoidUnusedParameters")] // for future compatibility | ||
| private void EnsureCanGetCol(string operationName, int ordinal) | ||
| private void EnsureCanGetCol(int ordinal, [CallerMemberName] string operationName = null) |
There was a problem hiding this comment.
Two things rule CA1801 has been deprecated in favor of IDE0060.
Second why do we need the unused variable int ordinal at all?
There was a problem hiding this comment.
I'll replace CA1801 with IDE0060. Why do we need that ordinal parameter? no idea, I'd have thought we'd want to report which column we failed to get but it doesn't currently do that and I'm not trying to fixup new issues just get the codebases merged.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDataReaderSmi.cs
Show resolved
Hide resolved
|
Also can you address the conflict please? |
Wraith2
force-pushed
the
clean-valueutils
branch
from
0be5754
to
205f715
Compare
|
the failures are related to your changes. Can you check that please? |
|
Done. I thought the CI wasn't working, it doesn't seem to run any of the version specific legs any more. |
A whole lot of style changes with one fix for a programming error which I'll highlight in the code.
In future it might be worth creating a bitmap like BitArray for the read write bool maps, they're currently quite wasteful of memory.