Skip to content
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

Cleanup PowerStatus interop #2041

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Oct 6, 2019

Proposed Changes

  • Cleanup GetPowerStatus
  • Cleanup SetPowerStatus
Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner October 6, 2019 09:31
@codecov
Copy link

codecov bot commented Oct 6, 2019

Codecov Report

Merging #2041 into master will increase coverage by 0.36525%.
The diff coverage is 20%.

@@                 Coverage Diff                 @@
##              master       #2041         +/-   ##
===================================================
+ Coverage   27.70556%   28.07081%   +0.36525%     
===================================================
  Files            879         880          +1     
  Lines         266701      266672         -29     
  Branches       37945       37945                 
===================================================
+ Hits           73891       74857        +966     
+ Misses        187585      186586        -999     
- Partials        5225        5229          +4
Flag Coverage Δ
#Debug 28.07081% <20%> (+0.36525%) ⬆️
#production 28.07081% <20%> (+0.36525%) ⬆️
#test 100% <ø> (ø) ⬆️

@RussKie RussKie added code cleanup cleanup code for unused apis/properties/comments - no functional changes. enhancement Product code improvement that does NOT require public API changes/additions 📭 waiting-author-feedback The team requires more information from the author labels Oct 8, 2019
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Oct 8, 2019

internal static class BooleanExtensions
{
public static bool IsTrue(this BOOLEAN b) => b != BOOLEAN.FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be more prudent to override the operators !, !=, and == for comparisons to both BOOLEAN and bools

Also, maybe I am mistaken, but isn't there already a BOOL which is identical to this? Is there no way to make both BOOLEAN and BOOL extend some common type which extends byte and then add these helper functions to that underlying type? Just trying to avoid duplicating code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't unless you make it a struct instead of an enum. It can be done per my example, it just isn't something the Core architects liked as it isn't technically "correct" and enums can potentially get better optimizations. It does work, however, and makes usage pretty easy, imho.

Copy link
Contributor

@weltkante weltkante Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there already a BOOL which is identical to this?

@zsd4yr no, BOOL and BOOLEAN are different sizes in the headers (which was the reason BOOLEAN is added here)

make it a struct instead of an enum [...] It does work, however

@JeremyKuhne only due to bug-for-bug compatibility with Desktop Framework, structs can trigger different behavior in calling conventions than primitives, passing them as a stack pointer instead of by-value in a register. Initially .NET Core caused crashes in WPF when using HRESULT struct wrappers as return values, they restored the bug from Desktop to keep this particular scenario compatible but I'm not entirely sure it will work in other situations, considering that the calling conventions can make differences between wrapper structs and wrapped values, so they are a bit of a minefield.

relevant discussions: [1] [2] [3]

Technical TLDR: the native calling conventions for returning structs in

  • SomeStruct(__stdcall SomeInterface::*SomeMethod)(); and
  • SomeStruct(__stdcall *SomeMethod)(SomeInterface*);

(i.e. passing this explicitely or implicitely) are not identical and .NET cannot differentiate between them, historically it has been mixed up due to a bug, and today (for compatibility) it has to guess looking at the struct which one is more likely what you meant, if it happens to chose the wrong case for your usage of the struct you crash or get garbage results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also please #1978 (comment)

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Oct 8, 2019
@RussKie RussKie merged commit 01238a9 into dotnet:master Oct 10, 2019
@ghost ghost added this to the 5.0 milestone Oct 10, 2019
@hughbe hughbe deleted the PowerStatus-Interop branch October 10, 2019 07:45
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code cleanup cleanup code for unused apis/properties/comments - no functional changes. enhancement Product code improvement that does NOT require public API changes/additions 📭 waiting-author-feedback The team requires more information from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants