Add CA2111 SuppressMessage for these MSDN documented public Pointers #4970
Conversation
| @@ -9,7 +9,8 @@ namespace System.Runtime.InteropServices.ComTypes | |||
| [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] | |||
| public struct TYPEDESC | |||
| { | |||
| public IntPtr lpValue; | |||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2111:PointersShouldNotBeVisible")] | |||
There was a problem hiding this comment.
Nit: these edits are all using tabs instead of spaces. CoreFX guidelines (and the rest of the file) use spaces.
| [FieldOffset(0)] | ||
| public IntPtr lpfuncdesc; | ||
| public IntPtr lpfuncdesc; // PUBLICLY DOCUMENTED lpfuncdesc field |
There was a problem hiding this comment.
It is not obvious that this comment applies to the SuppressMessage attribute.
It is better to include this as Justification = in the SuppressMessage attribute.
There was a problem hiding this comment.
Also, I do not think you need to use ALL CAPS for the justification. People frequently use just:
Justification = "Backwards compatibility"
There was a problem hiding this comment.
Fixed. These words were copied from others places..anyway, update them according to review comment
| @@ -9,11 +9,16 @@ namespace System.Runtime.InteropServices.ComTypes | |||
| [StructLayout(LayoutKind.Explicit, CharSet = CharSet.Unicode)] | |||
| public struct BINDPTR | |||
| { | |||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2111:PointersShouldNotBeVisible")] | |||
|
@luqunl Do you plan to finish this? |
|
@luqunl Ping |
| @@ -9,10 +9,13 @@ namespace System.Runtime.InteropServices.ComTypes | |||
| [StructLayout(LayoutKind.Explicit, CharSet = CharSet.Unicode)] | |||
| public struct BINDPTR | |||
| { | |||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2111:PointersShouldNotBeVisible", "Backwards compatibility")] | |||
There was a problem hiding this comment.
Nit: Add using System.Diagnostics.CodeAnalysis; to make the lines shorter?
|
... and build breaks. |
|
@luqunl Could you please "Squash and merge" next time? We don't really want "Merge branch master" type of commits in the master branch history. |
|
@MichalStrehovsky , do you mean that during code review, don't merge and after code review complete, squash and merge? |
|
The Merge button in the GitHub web UI has a small arrow next to it that lets you pick between Create merge commit, Squash and merge, and rebase and merge. For changes like this, use the squash option. |
|
Thanks a lot. Good to know this trick. |
No description provided.