search

dotnet / pinvoke

A library containing all P/Invoke code so you don't have to import it every time. Maintained and updated to support the latest Windows OS.
MIT License
2.12k stars 222 forks source link

WindowMessage enum underlying type should be uint #550

Closed zgabi closed 3 years ago

zgabi commented 3 years ago

WindowMessage enum underlying type should be uint.

Here: https://github.com/dotnet/pinvoke/blob/180d0363c200df1564a9e6508aa4d801522994f4/src/User32/User32%2BWindowMessage.cs#L18

See: https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/ms633573(v=vs.85) or: https://docs.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-cwpretstruct

vatsan-madhavan commented 3 years ago

App defined window messages max-out at 0xFFFF IIRC which should be well within the range of int's. Is there a sceanrio in which this can caused an actual bug?

uint is non-CLS compliant, so defining these things as int rather than uint for public types may actually be preferable.

zgabi commented 3 years ago

No, it is not causing any problem, just noticed that other structs (which is not available in this package) on pinvoke.net is using uint fields for the same value. For example: http://pinvoke.net/default.aspx/Structures/CWPRETSTRUCT.html

And other enums in this package are using uint for enums, for example: https://github.com/dotnet/pinvoke/blob/180d0363c200df1564a9e6508aa4d801522994f4/src/User32/User32%2BMessageBoxResult.cs

So I think it should be make consistent.

vatsan-madhavan commented 3 years ago

pinvoke.net is an similarly named but different site, I think.

You're right about some of these inconsistencies though.

It would be nice if we had some rule or heuristic for int/uint use, like 'prefer int over uint whenever it is safe'. Thoughts @AArnott ?

zgabi commented 3 years ago

Yes. pinvoke.net is different site. The mentioned CWPRETSRUCT is missing from this package, this is why I linked pinvoke.net. You can see there that the window message field is usually uint in .net. But you can check the microsoft site, too with the C++ definition. I thought better to link a C# example. https://docs.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-cwpretstruct.

AArnott commented 3 years ago

I thought we had it documented in our CONTRIBUTING.md file but I guess not. To improve interop with existing .NET APIs we generally prefer to use int over uint except where values that exceed int.MaxValue are expected. However in the case of an enum, it wouldn't make any difference so we probably should have gone with uint here. But again, as it actually makes no difference, now that it's done I think "fixing" it would be a binary breaking change with no benefit, so I'll close this as Won't Fix.

Thanks for raising awareness though, and for the good discussion.