Conversation
…rayToString, FormatMessage and FormatMessageFromModule()
| NativeMethods.ReadConsole( | ||
| consoleHandle.DangerousGetHandle(), | ||
| buffer, | ||
| out buffer, |
There was a problem hiding this comment.
Not sure why this was changed to an out parameter, since a valid object reference is passed in?
There was a problem hiding this comment.
It come from the method signature https://docs.microsoft.com/en-us/windows/console/readconsole
My intentions were to fix whole signatures, but it’s too much work and I stop this.
I can revert the change too.
There was a problem hiding this comment.
By pure coincidence, I noticed this change, and I don't think it's correct. See the last comment here. The original MSDN mag article seems to be gone, but the relevant part is quoted:
StringBuilder and Marshaling
The CLR marshaler has built-in knowledge of the StringBuilder type and treats it differently from other types. By default, StringBuilder is passed as [InAttribute, OutAttribute]. StringBuilder is special because it has a Capacity property that can determine the size of the required buffer at run time, and it can be changed dynamically. Therefore, during the marshaling process, the CLR can pin StringBuilder, directly pass the address of internal buffer used in StringBuilder, and allow the contents of this buffer to be changed by native code in place.
To take full advantage of StringBuilder, you'll need to follow all of these rules:
- Don't pass
StringBuilderby reference (usingoutorref). Otherwise, the CLR will expect the signature of this argument to bewchar_t **instead ofwchar_t *, and it won't be able to pinStringBuilder's internal buffer. Performance will be significantly degraded. - Use
StringBuilderwhen the unmanaged code is using Unicode. Otherwise, the CLR will have to make a copy of the string and convert it between Unicode and ANSI, thus degrading performance. Usually you should marshalStringBuilderasLPARRAYof Unicode characters or asLPWSTR. - Always specify the capacity of
StringBuilderin advance and make sure the capacity is big enough to hold the buffer. The best practice on the unmanaged code side is to accept the size of the string buffer as an argument to avoid buffer overruns. In COM, you can also usesize_isin IDL to specify the size.
In reply to: 255789200 [](ancestors = 255789200)
There was a problem hiding this comment.
@jazzdelightsme Thanks! I skipped that it is not buffer. Will revert.
Done #9066
| ); | ||
|
|
||
| [DllImport(PinvokeDllNames.ScrollConsoleScreenBufferDllName, SetLastError = true, CharSet = CharSet.Unicode)] | ||
| [return: MarshalAs(UnmanagedType.Bool)] |
There was a problem hiding this comment.
I understand adding these attributes to be consistent throughout our codebase, but why are they needed? It seems like 'marshal to bool' is implied...
There was a problem hiding this comment.
Staffan Gustafsson fixed a bug in one place (there is BOOLEAN not BOOL type in native code). I reviewed all such signatures and added the attribute to ensure that all right.
|
@PaulHigin Could you please continue the review? |
|
@PaulHigin Thanks for code review! |
PR Summary
PR Context
After moving to .Net Core 2 we migrate to ported APIs and now we have many unused Windows pinvokes.
Also
return: MarshalAs(UnmanagedType.Bool)was added where it was missing.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests