Skip to content

Conversation

@burakutkuc
Copy link

Related to #509

@mvandervoord
Copy link
Member

Hi. I saw your fix on Unity and, assuming it passes tests, will likely merge that.

This particular PR, though, I'm probably not going to merge. CMock has the ability for you to map these types to whatever you would prefer. The defaults represent the generally-preferred values after quite a few years of polling the community, etc. More developers preferred to see hex for their unsigned values than an unsigned int. So that's become the default.

@burakutkuc
Copy link
Author

Hi. I saw your fix on Unity and, assuming it passes tests, will likely merge that.

This particular PR, though, I'm probably not going to merge. CMock has the ability for you to map these types to whatever you would prefer. The defaults represent the generally-preferred values after quite a few years of polling the community, etc. More developers preferred to see hex for their unsigned values than an unsigned int. So that's become the default.

Hi Mark,

Thanks for your reply.

In the case where UINT32 is mapped to HEX32, the generated test uses UNITY_TEST_ASSERT_EQUAL_HEX32. But this macro casts the values to int32_t, which overflows when given UINT32_MAX.

Is this the intended behavior by default? Or am I missing something?

Thanks again!

@burakutkuc
Copy link
Author

Hi Mark,

If the current solution isn’t ideal, I can also fix the issue by modifying Unity itself.

Currently, UNITY_TEST_ASSERT_EQUAL_HEX32 looks like this:

#define UNITY_TEST_ASSERT_EQUAL_HEX32(expected, actual, line, message) \
    UnityAssertEqualIntNumber((UNITY_INT)(UNITY_INT32)(expected), (UNITY_INT)(UNITY_INT32)(actual), (message), (UNITY_LINE_TYPE)(line), UNITY_DISPLAY_STYLE_HEX32)

To prevent overflow when passing values like UINT32_MAX, I suggest updating it to use the unsigned version instead:

#define UNITY_TEST_ASSERT_EQUAL_HEX32(expected, actual, line, message) \
    UnityAssertEqualUintNumber((UNITY_UINT)(expected), (UNITY_UINT)(actual), (message), (UNITY_LINE_TYPE)(line), UNITY_DISPLAY_STYLE_HEX32)

Let me know if this approach would be acceptable.

@mvandervoord
Copy link
Member

Hi @burakutkuc -- yes, what you describe is the intended behavior! A hex comparison should be an unsigned comparison, just displayed differently. I think fixing it in Unity is the write thing to do.

@burakutkuc
Copy link
Author

Hi @burakutkuc -- yes, what you describe is the intended behavior! A hex comparison should be an unsigned comparison, just displayed differently. I think fixing it in Unity is the write thing to do.

Hi @mvandervoord,

I’ve made the improvements in Unity;
ThrowTheSwitch/Unity#784

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants