-
Couldn't load subscription status.
- Fork 1.1k
CA1838 Avoid 'StringBuilder' parameters for P/Invokes #8113
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.
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
Changes from 13 commits
496a47d
73ec2f9
b3860a2
199bfb5
26a3f15
84f87f2
4b51246
1af7290
fb238ea
09f3073
1a4999f
06ee405
01345e6
22c139e
3cd1ae7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Buffers; | ||
|
|
||
| namespace Windows.Win32 | ||
| { | ||
| internal static partial class PInvoke | ||
| { | ||
| public static unsafe string GetModuleFileNameLongPath(HINSTANCE hModule) | ||
| { | ||
| Span<char> buffer = stackalloc char[MAX_PATH]; | ||
elachlan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| uint pathLength; | ||
| fixed (char* lpFilename = buffer) | ||
| { | ||
| pathLength = GetModuleFileName(hModule, lpFilename, (uint)buffer.Length); | ||
| } | ||
|
|
||
| if (pathLength == 0) | ||
| { | ||
| return string.Empty; | ||
| } | ||
|
|
||
| if (pathLength < buffer.Length) | ||
elachlan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| return new string(buffer[..(int)pathLength]); | ||
| } | ||
|
|
||
| char[] lbuffer; | ||
| int bufferSize = 4096; | ||
| // Allocate increasingly larger portions of memory until successful or we hit short.maxvalue. | ||
| for (int i = 1; bufferSize <= short.MaxValue; i++, bufferSize = 4096 * i) | ||
| { | ||
| lbuffer = ArrayPool<char>.Shared.Rent(bufferSize); | ||
| fixed (char* lpFilename = lbuffer) | ||
| { | ||
| pathLength = GetModuleFileName(hModule, lpFilename, (uint)lbuffer.Length); | ||
| } | ||
|
|
||
| if (pathLength == 0) | ||
| { | ||
| ArrayPool<char>.Shared.Return(lbuffer); | ||
| return string.Empty; | ||
| } | ||
|
|
||
| // If the length equals the buffer size we need to check to see if we were told the buffer was insufficient (it was trimmed) | ||
| if (pathLength < lbuffer.Length) | ||
JeremyKuhne marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| // Get return value and return buffer to array pool. | ||
| string returnValue = new string(lbuffer, 0, (int)pathLength); | ||
| ArrayPool<char>.Shared.Return(lbuffer); | ||
| return returnValue; | ||
| } | ||
|
|
||
| // buffer was too small, return to array pool. | ||
| ArrayPool<char>.Shared.Return(lbuffer); | ||
| } | ||
|
|
||
| throw new InvalidOperationException(); | ||
elachlan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace Windows.Win32 | ||
| { | ||
| internal static partial class PInvoke | ||
| { | ||
| public static unsafe string GetWindowText(HWND hWnd) | ||
| { | ||
| // Old implementation uses MAX_TITLE_LENGTH characters | ||
| const int MAX_TITLE_LENGTH = 512; | ||
| int length; | ||
| Span<char> buffer = stackalloc char[MAX_TITLE_LENGTH]; | ||
| fixed (char* lpString = buffer) | ||
| { | ||
| length = GetWindowText(hWnd, (PWSTR)lpString, buffer.Length); | ||
| } | ||
|
|
||
| // If the window has no title bar or text, if the title bar is empty, | ||
| // or if the window or control handle is invalid, the return value is zero | ||
| return length == 0 ? string.Empty : new string(buffer[..length]); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace Windows.Win32 | ||
| { | ||
| internal static partial class PInvoke | ||
| { | ||
| /// <summary> | ||
| /// <para>The maximum length for lpszClassName is 256. If lpszClassName is greater than the maximum length, the RegisterClassEx function will fail.</para> | ||
| /// <para><see href="https://learn.microsoft.com/windows/win32/api/winuser/ns-winuser-wndclassexw#members">Read more on docs.microsoft.com</see>.</para> | ||
| /// </summary> | ||
| public const int MaxClassName = 256; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10584,7 +10584,7 @@ public void RichTextBox_CheckDefaultNativeControlVersions() | |
| using var control = new RichTextBox(); | ||
| control.CreateControl(); | ||
|
|
||
| Assert.Contains("RICHEDIT50W", GetClassName(control.Handle), StringComparison.InvariantCultureIgnoreCase); | ||
| Assert.Contains("RICHEDIT50W", GetClassName(control.HWND), StringComparison.Ordinal); | ||
| } | ||
|
|
||
| [WinFormsFact] | ||
|
|
@@ -10593,13 +10593,13 @@ public void RichTextBox_CheckRichEditWithVersionCanCreateOldVersions() | |
| using (var riched32 = new RichEdit()) | ||
| { | ||
| riched32.CreateControl(); | ||
| Assert.Contains(".RichEdit.", GetClassName(riched32.Handle), StringComparison.InvariantCultureIgnoreCase); | ||
| Assert.Contains(".RichEdit.", GetClassName(riched32.HWND), StringComparison.Ordinal); | ||
| } | ||
|
|
||
| using (var riched20 = new RichEdit20W()) | ||
| { | ||
| riched20.CreateControl(); | ||
| Assert.Contains(".RichEdit20W.", GetClassName(riched20.Handle), StringComparison.InvariantCultureIgnoreCase); | ||
| Assert.Contains(".RichEdit20W.", GetClassName(riched20.HWND), StringComparison.Ordinal); | ||
|
|
||
| string rtfString = @"{\rtf1\ansi{" + | ||
| @"The next line\par " + | ||
|
|
@@ -10615,8 +10615,8 @@ public void RichTextBox_CheckRichEditWithVersionCanCreateOldVersions() | |
| Assert.Equal(riched20.Text, richTextBox.Text); | ||
| Assert.Equal(richTextBox.Text.Length, richTextBox.TextLength); | ||
|
|
||
| int startOfIs = riched20.Text.IndexOf("is"); | ||
| int endOfHidden = riched20.Text.IndexOf("hidden") + "hidden".Length; | ||
| int startOfIs = riched20.Text.IndexOf("is", StringComparison.Ordinal); | ||
| int endOfHidden = riched20.Text.IndexOf("hidden", StringComparison.Ordinal) + "hidden".Length; | ||
| richTextBox.Select(startOfIs, endOfHidden - startOfIs); | ||
| Assert.Equal("is ###NOT### hidden", richTextBox.SelectedText); | ||
| } | ||
|
|
@@ -10791,12 +10791,16 @@ private class SubRichTextBox : RichTextBox | |
| public new void WndProc(ref Message m) => base.WndProc(ref m); | ||
| } | ||
|
|
||
| private static string GetClassName(IntPtr hWnd) | ||
| private static unsafe string GetClassName(HWND hWnd) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this move to the PInvoke namespace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there is another place where we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't move this because its a wrapper and isn't used in the other calls. I am happy to still move it. The other uses avoid string allocations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you think this is a dup, feel free to remove this. I can't recall exact reasons why I copied it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since its a wrapper of the pinvoke and we don't reuse it, I think it can stay here. |
||
| { | ||
| const int MaxClassName = 256; | ||
| StringBuilder sb = new StringBuilder(MaxClassName); | ||
| UnsafeNativeMethods.GetClassName(new HandleRef(null, hWnd), sb, MaxClassName); | ||
| return sb.ToString(); | ||
| int length = 0; | ||
| Span<char> buffer = stackalloc char[PInvoke.MaxClassName]; | ||
| fixed (char* lpClassName = buffer) | ||
| { | ||
| length = PInvoke.GetClassName(hWnd, lpClassName, buffer.Length); | ||
| } | ||
|
|
||
| return new string(buffer[..length]); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have worked but we are still getting build errors. I don't know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build is failing for other reasons:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it has the failures:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft.VisualBasic.Forms doesn't reference System.Windows.Forms.Primitives project directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and I personally don't think it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IVT is not transitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RussKie Did you want me to remove the
Microsoft.VisualBasic.Formschanges and we can work out what to do for it in a separate PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defer the decision to @lonitra and @JeremyKuhne.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elachan it's probably better to do the VB stuff separately for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it.