Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup SqlDataRecord #1133

Merged
merged 1 commit into from Jun 25, 2021
Merged

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Jun 23, 2021

Follow-up from #1024 , This implements the suggested and agreed changes from that PR feedback.

  1. remove EnsureSubclassOverride because there is no way to create an SqlDataRecord without the internal buffer that it is checking being set.
  2. Ensure that all callsites with an ordinal parameter directly or indirectly (through GetMetaData or GetSmiMetaData) call ThrowIfInvalidOrdinal
  3. remove the unused internal ctor
  4. mark most fields as readonly

@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v4.0 via automation Jun 24, 2021
@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview1 milestone Jun 24, 2021
SqlClient v4.0 automation moved this from In progress to Reviewer approved Jun 25, 2021
@cheenamalhotra cheenamalhotra merged commit a0a66a7 into dotnet:main Jun 25, 2021
SqlClient v4.0 automation moved this from Reviewer approved to Done Jun 25, 2021
@Wraith2 Wraith2 deleted the clean-sqldatarecord branch June 25, 2021 20:13
@TrayanZapryanov
Copy link
Contributor

@Wraith2 I've looked at netcore SqlDataRecord class and found several interesting things:

  1. Several things looks unused
    image
  2. The code in netcore SmiEventSink_Default class is quite strange - at least for me :) . I can't understand who will fill _errors and _warnings fields. If they are not used , maybe it is worth creating a static dummy class which does nothing in ProcessMessagesAndThrow.
  3. _usesStringStorageForXml seems to have a constant "true" value

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 3, 2021

  1. Yes, to the compiler in netcore builds. They might be used directly in the netfx build or used through reflection or inheritance. My current focus is on merging as much of the netcore and netfx codebases as possible and there are many peculiar or nonsensical things that will need resolving once that first task is finished.
  2. Yes, as far as I can tell it's never used and when using SqlDataRecord the message sink functions turn up in profiles. Again, with deeper investigation on both codebases they may be possible to dummy out as you said, I just haven't tried to go deep yet. If you want to do so it would be welcome.
  3. check if it's ever used in netfx, if not and there is no path to set it through inheritance then we could get rid of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants