Skip to content

Nullable annotations for System.Security.Cryptography.Xml#67198

Merged
krwq merged 67 commits intodotnet:mainfrom
SteveDunn:nrt-annotations
Sep 27, 2022
Merged

Nullable annotations for System.Security.Cryptography.Xml#67198
krwq merged 67 commits intodotnet:mainfrom
SteveDunn:nrt-annotations

Conversation

@SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Mar 27, 2022 •
edited
Loading

Fixes #67197

@ghost ghost added area-System.Security community-contribution Indicates that the PR has been added by a community member labels Mar 27, 2022
@ghost
Copy link

ghost commented Mar 27, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This is initially a draft pull request as there are a few questions that need answering as I go through

Author: SteveDunn
Assignees: -
Labels:

area-System.Security, community-contribution
Milestone: -

@SteveDunn SteveDunn changed the title Fixes #67197 nullable annotations for annotations for System.Security.Cryptography.Xml (DRAFT) Fixes #67197 nullable annotations for System.Security.Cryptography.Xml (DRAFT) Mar 27, 2022
@SteveDunn SteveDunn changed the title Fixes #67197 nullable annotations for System.Security.Cryptography.Xml (DRAFT) Nullable annotations for System.Security.Cryptography.Xml (DRAFT) Mar 27, 2022
@maxkoshevoi
Copy link
Contributor

maxkoshevoi commented Mar 27, 2022

If this is not ready for review yet, please make this PR an actual draft. There should be a button to do that:

image

@SteveDunn SteveDunn marked this pull request as draft March 27, 2022 11:58
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Mar 27, 2022

If this is not ready for review yet, please make this PR an actual draft. There should be a button to do that:

image

Thanks @maxkoshevoi - I was wondering where that option was. Hope you're keeping well and safe!

@maxkoshevoi
Copy link
Contributor

maxkoshevoi commented Mar 27, 2022

Thanks @maxkoshevoi - I was wondering where that option was. Hope you're keeping well and safe!

Thanks, I'm in relatively safe city, still have my job, roof over the head, and something to do in free time (walking around the city, and annotating runtime 😄), so things are quite good over all =)
Hope it'll be safe to return to my home in a month or so.

@SteveDunn SteveDunn changed the title Nullable annotations for System.Security.Cryptography.Xml (DRAFT) Nullable annotations for System.Security.Cryptography.Xml Mar 29, 2022
@SteveDunn SteveDunn marked this pull request as ready for review March 30, 2022 06:01
maxkoshevoi
maxkoshevoi reviewed Mar 30, 2022
View reviewed changes
@SteveDunn SteveDunn mentioned this pull request Mar 31, 2022
Closed
36 tasks
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Apr 23, 2022

I've just tidied up a few build errors after the recent rebase from main (after all the !!'s were removed).

I'm getting strange errors such as:

1>C:\git\runtime\src\libraries\Common\src\Interop\Windows\Advapi32\Interop.CryptAcquireContext.cs(25,36,25,55): error CA1420: Setting SetLastError to 'true' requires runtime marshalling to be enabled
1>C:\git\runtime\src\libraries\Common\src\Interop\Windows\Advapi32\Interop.CryptAcquireContext.cs(26,32,26,38): error CA1420: By-ref parameters require runtime marshalling to be enabled

Any ideas?

@SteveDunn SteveDunn mentioned this pull request Sep 3, 2022
Closed
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Sep 3, 2022

LGTM after resolving comments and merge conflicts

I've rebased and looked at the issues you've mentioned.

krwq
krwq approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

looks good, there seem to be couple of unaddressed comments

@krwq
Copy link
Member

krwq commented Sep 8, 2022

Please do not rebase unless there are merge conflicts. Just push changes with those three fixes (or respond to the comments if not addressing for whatever reason)

@SteveDunn
Copy link
Contributor Author

SteveDunn commented Sep 13, 2022

Please do not rebase unless there are merge conflicts. Just push changes with those three fixes (or respond to the comments if not addressing for whatever reason)

I don't rebase unless there are merge conflicts. Unfortunately, last time, there were merge conflicts. I think I've done 4 or 5 rebases for this PR!

@SteveDunn
Copy link
Contributor Author

SteveDunn commented Sep 15, 2022

Hi - I think everything is now addressed here. Might we be able to merge it before a rebase is due again? :)

@SteveDunn
Copy link
Contributor Author

SteveDunn commented Sep 22, 2022

Hi again, can this be merged/closed before another rebase is required? Thanks in advance.

@stephentoub
Copy link
Member

stephentoub commented Sep 26, 2022

@bartonjs, can you help land this?

krwq
krwq approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

Looks good from my side. Thank you!

@krwq krwq merged commit 0075639 into dotnet:main Sep 27, 2022
@krwq
Copy link
Member

krwq commented Sep 27, 2022

Contributes to: #41720

@stephentoub
Copy link
Member

stephentoub commented Sep 27, 2022

Thank you, @SteveDunn.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@bartonjs bartonjs bartonjs left review comments

@krwq krwq krwq approved these changes

+2 more reviewers

@am11 am11 am11 left review comments

@maxkoshevoi maxkoshevoi maxkoshevoi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@krwq krwq

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Group 5] Enable nullable annotations for System.Security.Cryptography.Xml

9 participants

@SteveDunn @maxkoshevoi @bartonjs @stephentoub @krwq @ViktorHofer @danmoseley @vcsjones @am11