8000 Remove 'out' in ReadConsole pinvoke by iSazonov · Pull Request #9066 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Remove 'out 8000 ' in ReadConsole pinvoke#9066

Merged
daxian-dbw merged 1 commit intoPowerShell:masterfrom
iSazonov:revert-out-in-readconsole
Mar 14, 2019
Merged

Remove 'out' in ReadConsole pinvoke#9066
daxian-dbw merged 1 commit intoPowerShell:masterfrom
iSazonov:revert-out-in-readconsole

Conversation

@iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Mar 5, 2019

PR Summary

Revert change based on the comment #8847 (comment)

PR Context

PR Checklist

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Mar 5, 2019
@iSazonov iSazonov mentioned this pull request Mar 5, 2019
Merged
11 tasks
@iSazonov iSazonov changed the title Remove out in ReadConsole Remove 'out' in ReadConsole pinvoke Mar 5, 2019
@iSazonov iSazonov requested a review from PaulHigin March 6, 2019 05:20
@daxian-dbw
Copy link
Member
daxian-dbw commented Mar 13, 2019 •
edited
Loading

Look goo to me.
Can you please update this signature as well: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L7082
Should be changed from [return: MarshalAs(UnmanagedType.I1)] to [return: MarshalAs(UnmanagedType.Bool)].

No change is needed as per @jazzdelightsme pointed out below.

@jazzdelightsme
Copy link
Contributor
jazzdelightsme commented Mar 13, 2019 •
edited
Loading

@daxian-dbw : No! CreateSymbolicLinkW really does return a 1-byte boolean value. (BOOLEAN versus BOOL).

(and I verified this by looking at type information in a debugger; I am very sure)

@daxian-dbw
Copy link
Member
daxian-dbw commented Mar 13, 2019 •
edited
Loading

@jazzdelightsme Thanks for catching that! Can you please explain a bit more about the difference between BOOLEAN and BOOL in C++?
Is BOOL actually defined as an int in some header files?

@daxian-dbw
Copy link
Member
daxian-dbw commented Mar 13, 2019

Ha, I might find it -- https://stackoverflow.com/a/6420546/7129704

BOOL is a Microsoft specific type that is defined as an int. You can find it in windef.h

@daxian-dbw daxian-dbw merged commit 0f7efdd into PowerShell:master Mar 14, 2019
@iSazonov
Copy link
Collaborator Author
iSazonov commented Mar 14, 2019

@daxian-dbw I see in https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-pinvokes.md#strings that StringBuilder marshalling always re-allocate/re-copy buffers. Make sense review our code to reduce such allocations? Although at first glance I did not see how we could do it.

And the PR fix could be in 6.2.

@iSazonov iSazonov deleted the revert-out-in-readconsole branch March 14, 2019 03:15
@daxian-dbw
Copy link
Member
daxian-dbw commented Mar 14, 2019

Thanks @iSazonov. By reading that guideline, it seems StringBuilder should never be used for P/Invoke as it incurs 4 allocations. I think it make sense to review our uses of it and replace with ArrayPool<char> when appropriate, because we will copy the existing pattern when writing new code. I will take a look at this ReadConsole case in particular.

@iSazonov iSazonov mentioned this pull request Mar 14, 2019
Closed
@iSazonov
Copy link
Collaborator Author
iSazonov commented Mar 14, 2019

Tracking issue #9139

I will take a look at this ReadConsole case in particular.

Please review my old PR first - it is related :-)

@daxian-dbw
Copy link
Member
daxian-dbw commented Mar 15, 2019

@iSazonov We are using StringBuilder for the [out] string parameter in P/Invoke almost everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

Assignees

@daxian-dbw daxian-dbw

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iSazonov @daxian-dbw @jazzdelightsme
0