Skip to content

Optimize the SqlGuid struct#72549

Merged
jeffhandley merged 15 commits intodotnet:mainfrom
MichalPetryka:sqlguid
Jan 30, 2023
Merged

Optimize the SqlGuid struct#72549
jeffhandley merged 15 commits intodotnet:mainfrom
MichalPetryka:sqlguid

Conversation

@MichalPetryka
Copy link
Contributor

@MichalPetryka MichalPetryka commented Jul 20, 2022

Optimizes the SqlGuid struct according to the idea
from #51836.

@MichalPetryka
Optimize the SqlGuid struct
e07c937 
Optimizes the SqlGuid struct according to the idea
from dotnet#51836.
@ghost ghost added area-System.Data.SqlClient community-contribution Indicates that the PR has been added by a community member labels Jul 20, 2022
@ghost
Copy link

ghost commented Jul 20, 2022

Tagging subscribers to this area: @cheenamalhotra, @David-Engel
See info in area-owners.md if you want to be subscribed.

Issue Details

Optimizes the SqlGuid struct according to the idea
from #51836.

Author: MichalPetryka
Assignees: -
Labels:

area-System.Data.SqlClient
Milestone: -

@MichalPetryka MichalPetryka mentioned this pull request Jul 20, 2022
Closed
GrabYourPitchforks
GrabYourPitchforks reviewed Jul 20, 2022
View reviewed changes
src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLGuid.cs Outdated
void ISerializable.GetObjectData(SerializationInfo si, StreamingContext sc)
{
ArgumentNullException.ThrowIfNull(si);
si.AddValue("m_value", ToByteArray());
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the behavior of ToByteArray is reverted, this will have to change.

Wraith2
Wraith2 reviewed Jul 20, 2022
View reviewed changes
GrabYourPitchforks
GrabYourPitchforks reviewed Jul 21, 2022
View reviewed changes
@Wraith2 Wraith2 mentioned this pull request Jul 23, 2022
Merged
@ghost ghost closed this Sep 11, 2022
@ghost
Copy link

ghost commented Sep 11, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@MichalPetryka
Copy link
Contributor Author

MichalPetryka commented Sep 11, 2022

@David-Engel The bot closed this, could it be reopened?

@stephentoub stephentoub reopened this Sep 11, 2022
@ghost
Copy link

ghost commented Oct 12, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost closed this Oct 12, 2022
@teo-tsirpanis teo-tsirpanis reopened this Oct 18, 2022
@ghost ghost closed this Nov 17, 2022
@ghost
Copy link

ghost commented Nov 17, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@teo-tsirpanis teo-tsirpanis reopened this Nov 17, 2022
@MichalPetryka MichalPetryka marked this pull request as ready for review November 17, 2022 23:10
@MichalPetryka
Copy link
Contributor Author

MichalPetryka commented Nov 17, 2022

@David-Engel @Wraith2 So what's the status on this?

@build-analysis build-analysis bot mentioned this pull request Nov 18, 2022
Open
David-Engel
David-Engel approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this fell off my radar in draft so long and the last mention got buried under other emails. I'm good with the changes.

@David-Engel
Copy link
Contributor

David-Engel commented Dec 13, 2022

If @GrabYourPitchforks is good with the changes, they can be merged.

@danmoseley
Copy link
Member

danmoseley commented Jan 23, 2023

@jeffhandley is there anyone that can help look at this one

jeffhandley
jeffhandley requested changes Jan 24, 2023
View reviewed changes
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 24, 2023
@build-analysis build-analysis bot mentioned this pull request Jan 29, 2023
Closed
jeffhandley
jeffhandley approved these changes Jan 30, 2023
View reviewed changes
@jeffhandley
Copy link
Member

jeffhandley commented Jan 30, 2023

The failing CI leg is unrelated:

@jeffhandley jeffhandley merged commit bf5afd4 into dotnet:main Jan 30, 2023
@jeffhandley
Copy link
Member

jeffhandley commented Jan 30, 2023

Thank you for this contribution, @MichalPetryka! This change will be included in .NET 8 Preview 1.

@jeffhandley jeffhandley added this to the 8.0.0 milestone Jan 30, 2023
@Wraith2 Wraith2 mentioned this pull request Feb 22, 2023
Closed
1 task
@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@David-Engel David-Engel David-Engel approved these changes

+3 more reviewers

@madelson madelson madelson left review comments

@Rob-Hague Rob-Hague Rob-Hague left review comments

@Wraith2 Wraith2 Wraith2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Data.SqlClient community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

10 participants

@MichalPetryka @David-Engel @danmoseley @jeffhandley @madelson @GrabYourPitchforks @Rob-Hague @Wraith2 @stephentoub @teo-tsirpanis