terrafx / terrafx.interop.windows

Interop bindings for Windows.
MIT License
260 stars 31 forks source link

[IMPL] Add implicit conversion from RECT -> System.Drawing.Rectangle. #388

Closed AraHaan closed 2 months ago

AraHaan commented 8 months ago

Proposal Implementation

Adds an implicit conversion operator from RECT -> Rectangle with an explicit operator from Rectangle back to RECT when the rectangle needs converted to an RECT for calls into Windows APIs that requires it. Also with operators handling these conversions programmers would worry less on if they got their conversion between RECT <-> Rectangle correct or not by eliminating all the guess work from it. Unit tests of the operators will come soon. As such I opened this pull request as a draft.

Implements/Fixes https://github.com/terrafx/terrafx.interop.windows/issues/387.

tannergooding commented 8 months ago

Same general feedback as given on the issue. I'm not particularly a fan of this, as its potentially lossy and brings in a new dependency.

This really seems like something where a user could provide extension methods for their own library to solve the need.

rickbrew commented 2 months ago

Same general feedback as given on the issue. I'm not particularly a fan of this, as its potentially lossy and brings in a new dependency.

This really seems like something where a user could provide extension methods for their own library to solve the need.

I'm actually strongly opposed to this pull request. I do not think it belongs in TerraFX, and it would actually be a bug hazard in my own codebase. RECT and Rectangle are not interchangeable, they simply cannot represent the same values.

For instance, Rectangle cannot encode a rectangle with a width greater than int.MaxValue, while RECT can. Any RECT where right - left is greater than int.MaxValue will have silent overflows or worse. I have seen this sort of thing cause major problems in my own code base, it is not a theoretical problem. I have a lot of interop and conversions between "XYWH" rectangles (Rectangle, WICRect) and "LTRB" rectangles (RECT, D2D_RECT, etc). It is not something you can correctly handle with some implicit casting operators.

AraHaan commented 2 months ago

Same general feedback as given on the issue. I'm not particularly a fan of this, as its potentially lossy and brings in a new dependency. This really seems like something where a user could provide extension methods for their own library to solve the need.

I'm actually strongly opposed to this pull request. I do not think it belongs in TerraFX, and it would actually be a bug hazard in my own codebase. RECT and Rectangle are not interchangeable, they simply cannot represent the same values.

For instance, Rectangle cannot encode a rectangle with a width greater than int.MaxValue, while RECT can. Any RECT where right - left is greater than int.MaxValue will have silent overflows or worse. I have seen this sort of thing cause major problems in my own code base, it is not a theoretical problem. I have a lot of interop and conversions between "XYWH" rectangles (Rectangle, WICRect) and "LTRB" rectangles (RECT, D2D_RECT, etc). It is not something you can correctly handle with some implicit casting operators.

I guess I should have the operators check for if > int.MaxValue on right - left and if so, do something about it, Issue is what should be done in the case of right - left? Throw, or do some other type of logic to make it work in a way that won't make Rectangle throw? I never thought that right - left in TerraFX could possibly be > int.MaxValue due to each of them being int's.

rickbrew commented 2 months ago

Yup. If right is int.MaxValue and left is int.MinValue, then width should be uint.MaxValue, but that will not be representable as a (signed) int and will overflow. There are actually a substantial number of corner cases to handle when working with arbitrary rectangles. I still find bugs in my handling of RECT vs. Rectangle in Paint.NET even after 20 years. They can be very subtle and difficult to diagnose and fix.

I think @tannergooding has the right idea here. You should have your own utility or extension methods to handle the conversion, and it should not be baked into TerraFX.

The answer to "what should be done if ..." is going to be application specific. In some cases you want to throw an exception or do a no-op. In others you want to use a special value for "empty" or "logically infinite" rectangles. I think having operators baked into TerraFX would lock in one of these choices and make things a lot harder for anyone who needs the other ways of handling this.

Put another way: there are too many ways to get this very wrong, and I don't see any way to get this right for all applications.

tannergooding commented 2 months ago

Going to close this. Rick was able to reiterate the points I intended.