Ensure XML doc comments exist for the nested classes and IsSupported properties for intrinsics#106091
Conversation
ghost
added
the
|
CC. @carlossanlop |
|
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? |
…properties for intrinsics
tannergooding
force-pushed
the
fix-83165
branch
from
c1b51f9 to
1bbdced
Compare
gewarren
left a comment
gewarren
left a comment
There was a problem hiding this comment.
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
|
Updated to include the feedback |
|
CC. @carlossanlop, @gewarren could this get sign-off so it can be merged? |
|
Ping. @carlossanlop, @gewarren could this get sign-off so it can be merged? |
| { | ||
| internal Aes() { } | ||
|
|
||
| /// <summary>Gets a value that indicates whether the APIs in this class are supported.</summary> |
There was a problem hiding this comment.
I think this should instead say something along the lines of "Always returns false, indicating that the APIs in this class are not supported."
There was a problem hiding this comment.
Same suggestion for all the similar *.NotSupported.cs IsSupported properties that return false.
There was a problem hiding this comment.
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
left a comment
gewarren
left a comment
There was a problem hiding this comment.
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
|
@carlossanlop, do these need to be backported to resolve the 9.0 issue? |
3 participants
This resolves #83165