Make use of bx::bit_cast #3212

phniix · 2023-12-02T18:12:37Z

Use bx::bit_cast() where appropriate for type punning, and applying packed struct for arrays when necessary.

bx::bit_cast() is introduced in bkaradzic/bx#313 to address the similar issue of type punning in bx (bkaradzic/bx#312).

~~This PR also uses reinterpret_cast for direct void* translations for integer handles.~~
~~Within renderer_vk.cpp, we remove the union when casting from void* to xcb_window_t, it isn't necessary.~~

bkaradzic · 2024-02-10T02:39:56Z

My suggestion is to reduce do smaller PR, where you use only bx::bit_cast.

For example, you can replace this castfu function with bx::bit_cast, not implementation of it, rather function itself:

	inline uint32_t castfu(float _value)
	{
		union {	float fl; uint32_t ui; } un;
		un.fl = _value;
		return un.ui;
	}

phniix · 2024-02-10T17:26:12Z

My suggestion is to reduce do smaller PR, where you use only bx::bit_cast.

For example, you can replace this castfu function with bx::bit_cast, not implementation of it, rather function itself:
	inline uint32_t castfu(float _value)
	{
		union {	float fl; uint32_t ui; } un;
		un.fl = _value;
		return un.ui;
	}

Sure yes, no worries. Let me know if the latest pushes are what you had in mind.

bkaradzic · 2024-02-10T19:17:57Z

examples/common/entry/entry_sdl.cpp

@@ -830,7 +825,7 @@ namespace entry
 							{
 							case SDL_USER_WINDOW_CREATE:
 								{
-									WindowHandle handle = getWindowHandle(uev);
+									WindowHandle handle = reinterpret_cast<WindowHandle>(_uev.data1);


I prefer you leave getWindowHandle instead this reinterpret_cast. Basically ideally no reinterpret_cast anywhere.

Sure thing!

bkaradzic · 2024-02-12T01:08:52Z

examples/common/entry/entry_sdl.cpp

-		union { void* p; WindowHandle h; } cast;
-		cast.h = _handle;
-		uev.data1 = cast.p;
+		uev.data1 = reinterpret_cast<void*>(_handle);


One benefit of union here is that type-safety will actually error if handle doesn't match. Where reinterpret_cast will just do whatever.

You bring up an excellent point about the importance of type checking. It's crucial to ensure stability, especially in scenarios where _handle might change its type unexpectedly. For instance, if the API were to transition WindowHandle to an integer type like uint64_t, it could lead to complications if the cast to void* isn't adjusted accordingly.

I think we can enhance our type checking to be more robust and explicit by incorporating bx::isSame<Ty, Uy>(). This approach emphasizes clarity and intentionality, making it easier to understand the expected behavior. I'll demonstrate this further with an example in an upcoming commit.

bkaradzic

In almost every use of reinterpret_cast in your PR you're making code worse than before IMO. Not sure what's your goal here?!

bkaradzic · 2024-02-24T18:43:18Z

examples/09-hdr/hdr.cpp

-				union { uint32_t color; uint8_t bgra[4]; } cast = { m_lumBgra8 };
-				float exponent = cast.bgra[3]/255.0f * 255.0f - 128.0f;
-				float lumAvg   = cast.bgra[2]/255.0f * bx::exp2(exponent);
+				struct packed { uint8_t bgra[4]; } arr = bx::bit_cast<packed>(m_lumBgra8);


Code style is uppercase for struct/class, so Packed.

bkaradzic · 2024-02-24T18:44:07Z

examples/17-drawstress/drawstress.cpp

-
-			cast.ptr = _thread->pop();
-			if (UINTPTR_MAX == cast.id)
+			uintptr_t id = reinterpret_cast<uintptr_t>(_thread->pop());


Still don't like reinterpret cast... This is not better than union thing.

I should have probably marked the current this variable const in the commit. The difference we'd have is that we can mark uintptr_t id const, but not the same with union {} cast.

bkaradzic · 2024-02-24T18:45:45Z

examples/common/entry/entry_sdl.cpp

-		union { void* p; WindowHandle h; } cast;
-		cast.h = _handle;
-		uev.data1 = cast.p;
+		BX_STATIC_ASSERT((bx::isSame<WindowHandle, decltype(_handle)>()), "_handle must be of the type WindowHandle");


This is getting more complicated than union cast now...

This approach emphasizes clarity and intentionality, making it easier to understand the expected behavior.

The added type checking is a nice insurance policy in the event someone might inadvertently change the type of the input parameter. I wonder if we can get away with not having any such insurance policy, as it seems very likely that any type change to _handle would force a rethink of what is being stored and retrieved from SDL_UserEvent.data1..

bkaradzic · 2024-02-24T18:46:16Z

examples/common/entry/entry_x11.cpp

-			cast c;
-			c.w = window;
-			m_eventQueue.postWindowEvent(_handle, c.p);
+			m_eventQueue.postWindowEvent(_handle, reinterpret_cast<void*>(window));


Don't like reinterpret cast here.

bkaradzic · 2024-02-24T18:48:00Z

src/renderer_d3d12.cpp

@@ -2952,7 +2952,7 @@ namespace bgfx { namespace d3d12

 		static void patchCb0(DxbcInstruction& _instruction, void* _userData)
 		{
-			union { void* ptr; uint32_t offset; } cast = { _userData };
+			const uint32_t offset = uint32_t(reinterpret_cast<uintptr_t>(_userData));


You need to realize that I consciously didn't want reinterpret_cast when I wrote this code. I was aware that reinterpret_cast exist before...

It isn't something that appears super clear. As a reader, and user, I'm interpreting the usage as a reflex action (for example when we see the usage on line 3159), kind of like a C in C++ approach. Thank you for sharing your thought and decision process.

phniix

Not sure what's your goal here?!

I had thought that the PR title and description were clear in stating the goal. Apologies if they haven't been clear enough.

I'll update this PR to reflect only the bx::bit_cast changes, as there seems to be a strong desire to maintain the stylistic choice of using unions for pointer to integer conversions.

phniix · 2024-02-24T22:48:54Z

examples/common/entry/entry_sdl.cpp

-		union { void* p; WindowHandle h; } cast;
-		cast.h = _handle;
-		uev.data1 = cast.p;
+		BX_STATIC_ASSERT((bx::isSame<WindowHandle, decltype(_handle)>()), "_handle must be of the type WindowHandle");


This approach emphasizes clarity and intentionality, making it easier to understand the expected behavior.

The added type checking is a nice insurance policy in the event someone might inadvertently change the type of the input parameter. I wonder if we can get away with not having any such insurance policy, as it seems very likely that any type change to _handle would force a rethink of what is being stored and retrieved from SDL_UserEvent.data1..

phniix · 2024-02-24T22:59:48Z

src/renderer_d3d12.cpp

@@ -2952,7 +2952,7 @@ namespace bgfx { namespace d3d12

 		static void patchCb0(DxbcInstruction& _instruction, void* _userData)
 		{
-			union { void* ptr; uint32_t offset; } cast = { _userData };
+			const uint32_t offset = uint32_t(reinterpret_cast<uintptr_t>(_userData));


It isn't something that appears super clear. As a reader, and user, I'm interpreting the usage as a reflex action (for example when we see the usage on line 3159), kind of like a C in C++ approach. Thank you for sharing your thought and decision process.

phniix · 2024-02-24T23:08:15Z

examples/17-drawstress/drawstress.cpp

-
-			cast.ptr = _thread->pop();
-			if (UINTPTR_MAX == cast.id)
+			uintptr_t id = reinterpret_cast<uintptr_t>(_thread->pop());


I should have probably marked the current this variable const in the commit. The difference we'd have is that we can mark uintptr_t id const, but not the same with union {} cast.

…struct for arrays when necessary

…struct for arrays when necessary (bkaradzic#3212)

phniix requested a review from bkaradzic as a code owner December 2, 2023 18:12

bkaradzic requested changes Feb 10, 2024

View reviewed changes

phniix requested a review from bkaradzic February 11, 2024 19:49

bkaradzic requested changes Feb 12, 2024

View reviewed changes

phniix requested a review from bkaradzic February 24, 2024 14:55

bkaradzic requested changes Feb 24, 2024

View reviewed changes

phniix commented Feb 24, 2024

View reviewed changes

phniix changed the title ~~Remove type punning using unions~~ Make use of bx::bit_cast Feb 24, 2024

Use bx::bit_cast where appropriate for type punning, applying packed …

746be2c

…struct for arrays when necessary

phniix requested a review from bkaradzic February 24, 2024 23:29

bkaradzic approved these changes Feb 24, 2024

View reviewed changes

bkaradzic merged commit 0b049d4 into bkaradzic:master Feb 24, 2024
11 checks passed

mipek pushed a commit to mipek/bgfx that referenced this pull request Mar 2, 2024

Use bx::bit_cast where appropriate for type punning, applying packed …

5d66973

…struct for arrays when necessary (bkaradzic#3212)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make use of bx::bit_cast #3212

Make use of bx::bit_cast #3212

phniix commented Dec 2, 2023 •

edited

Loading

bkaradzic commented Feb 10, 2024

phniix commented Feb 10, 2024

bkaradzic Feb 10, 2024

phniix Feb 11, 2024

bkaradzic Feb 12, 2024

phniix Feb 18, 2024

bkaradzic left a comment •

edited

Loading

bkaradzic Feb 24, 2024

bkaradzic Feb 24, 2024

phniix Feb 24, 2024

bkaradzic Feb 24, 2024

phniix Feb 24, 2024

bkaradzic Feb 24, 2024

bkaradzic Feb 24, 2024

phniix Feb 24, 2024

phniix left a comment

phniix Feb 24, 2024

phniix Feb 24, 2024

phniix Feb 24, 2024

Make use of bx::bit_cast #3212

Make use of bx::bit_cast #3212

Conversation

phniix commented Dec 2, 2023 • edited Loading

bkaradzic commented Feb 10, 2024

phniix commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkaradzic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phniix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phniix commented Dec 2, 2023 •

edited

Loading

bkaradzic left a comment •

edited

Loading