-
Notifications
You must be signed in to change notification settings - Fork 1k
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
CA1838 Avoid 'StringBuilder' parameters for P/Invokes #8113
Merged
Merged
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
496a47d
Initial work to remove string builders from PInvoke
elachlan 73ec2f9
Fix build errors
elachlan b3860a2
changes from review
elachlan 199bfb5
more changes from review
elachlan 26a3f15
fix usings
elachlan 84f87f2
Changes from review
elachlan 4b51246
changes from review
elachlan 1af7290
Convert GetWindowText usage
elachlan fb238ea
changes from review to avoid char[] allocation
elachlan 09f3073
changes from review
elachlan 1a4999f
more changes from review
elachlan 06ee405
more changes from review
elachlan 01345e6
remove and sort usings
elachlan 22c139e
remove vb changes
elachlan 3cd1ae7
changes from review
elachlan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
63 changes: 63 additions & 0 deletions
63
src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.GetModuleFileNameLongPath.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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.
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); | ||
} | ||
|
||
return string.Empty; | ||
elachlan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} |
62 changes: 62 additions & 0 deletions
62
src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.GetWindowText.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// 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 | ||
{ | ||
//const int MAX_TITLE_LENGTH = 511; | ||
|
||
public static unsafe string GetWindowText(HWND hWnd) | ||
{ | ||
int length = GetWindowTextLength(hWnd); | ||
|
||
// If the window has no text, the return value is zero. | ||
if (length == 0) | ||
{ | ||
return string.Empty; | ||
} | ||
|
||
// Stackalloc for smaller values | ||
if (length <= 1024) | ||
{ | ||
Span<char> buffer = stackalloc char[length + 1]; | ||
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 | ||
if (length == 0) | ||
{ | ||
return string.Empty; | ||
} | ||
} | ||
|
||
// Use arraypool for larger than 1024 characters. | ||
char[] lBuffer; | ||
lBuffer = ArrayPool<char>.Shared.Rent(length + 1); | ||
elachlan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int pathLength; | ||
fixed (char* lpString = lBuffer) | ||
{ | ||
pathLength = GetWindowText(hWnd, (PWSTR)lpString, lBuffer.Length); | ||
elachlan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// 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 | ||
if (pathLength == 0) | ||
{ | ||
ArrayPool<char>.Shared.Return(lBuffer); | ||
return string.Empty; | ||
} | ||
|
||
// Get return value and return buffer to array pool. | ||
string returnValue = new string(lBuffer, 0, pathLength); | ||
ArrayPool<char>.Shared.Return(lBuffer); | ||
return returnValue; | ||
} | ||
} | ||
} |
15 changes: 15 additions & 0 deletions
15
src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.MaxClassName.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.Forms
changes 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.