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.

Sign up for GitHub

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

Jump to bottom

Ensure XML doc comments exist for the nested classes and IsSupported properties for intrinsics #106091

Merged
merged 3 commits into from
Aug 19, 2024
Merged

Ensure XML doc comments exist for the nested classes and IsSupported properties for intrinsics #106091

merged 3 commits into from
Aug 19, 2024

Conversation

tannergooding
Copy link
Member

This resolves #83165

@tannergooding
Copy link
Member Author

CC. @carlossanlop

@carlossanlop
Copy link
Member

Thanks @tannergooding ! So it turns out we were just missing the actual xml docs in triple slash. I thought the reported bug was something in the way the xmls were getting generated, or the porting tool not being capable of finding nested types.

@gewarren could you help with a language review?

This was referenced Aug 7, 2024
Closed
Open
Open
Closed
Open
Open
Open
Closed
gewarren
gewarren reviewed Aug 9, 2024
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

I left a few suggestions that looks like they could be applied throughout if you like them.

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.cs Outdated Show resolved Hide resolved
@tannergooding
Copy link
Member Author

Updated to include the feedback

@build-analysis build-analysis bot mentioned this pull request Aug 12, 2024
Closed
@tannergooding
Copy link
Member Author

tannergooding commented Aug 12, 2024 •
edited
Loading

CC. @carlossanlop, @gewarren could this get sign-off so it can be merged?

@tannergooding
Copy link
Member Author

Ping. @carlossanlop, @gewarren could this get sign-off so it can be merged?

carlossanlop
carlossanlop reviewed Aug 16, 2024
View reviewed changes
...braries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Aes.PlatformNotSupported.cs
/// </summary>
[CLSCompliant(false)]
public abstract class Aes : ArmBase
{
internal Aes() { }

/// <summary>Gets a value that indicates whether the APIs in this class are supported.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should instead say something along the lines of "Always returns false, indicating that the APIs in this class are not supported."

Copy link
Member

Choose a reason for hiding this comment

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

Same suggestion for all the similar *.NotSupported.cs IsSupported properties that return false.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have 1 set of docs that exist, not separate ones for *.cs for *.PlatformNotSupported.cs

The *.PlatformNotSupported.cs are purely implementation details for corelib to be more optimal

gewarren
gewarren approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

LGTM except for a handful of missing periods in the summary. You could find/replace intrinsics</summary> and also when the closing summary tag is on the following line.

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Dp.cs Outdated Show resolved Hide resolved
This was referenced Aug 18, 2024
Closed
Closed
@tannergooding tannergooding merged commit 19b5242 into dotnet:main Aug 19, 2024
145 of 147 checks passed
@tannergooding tannergooding deleted the fix-83165 branch August 19, 2024 03:17
@tannergooding
Copy link
Member Author

@carlossanlop, do these need to be backported to resolve the 9.0 issue?

@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@gewarren gewarren gewarren approved these changes

Assignees

@tannergooding tannergooding

Labels
area-System.Runtime.Intrinsics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nested classes in System.Runtime.Intrinsics are missing xml doc comments
3 participants
@tannergooding @carlossanlop @gewarren