LONGS_EQUAL and friends are not portable between Windows and Linux

[CiderMan]

LONGS_EQUAL and friends are not portable between Windows and Linux

[thetic]

None of C/C++'s fundamental integral types are portable¹. This is a limitation of the language, not CppUTest. By the logic of your first suggestion, we should allow comparisons of int, short, and char to be removed as well. I'm assuming your second suggestion would depend on templates, which I believe @basvodde is against. We might consider support for fixed width integer types introduced in C++11² and C99³ respectively.

Footnotes

1. https://en.cppreference.com/w/cpp/language/types#Signed_and_unsigned_integer_types ↩

2. https://en.cppreference.com/w/cpp/types/integer ↩

3. https://en.cppreference.com/w/c/types/integer ↩

[CiderMan]

Hi @thetic,

You are, of course, right. I've worked on many different systems in terms of type sizes over the years, including one where both int and char where 24-bit (so sizeof (int) was 1)!

I should have been clearer that I am talking about between Windows and Linux (clearly not the only use-cases but quite possibly the most likely to require portability...) I will update the title to be clearer.

Fixed width macros would certainly work for us but we do want to be able to remove the LONGS_EQUAL et al. from the API to avoid a whole class of portability bugs for our use-case.

Regarding CHECK_EQUAL, I tried the following, which uses type inference rather then explicit templates to avoid evaluating the arguments more than once, and it seems to work as a drop in replacement for LONGS_EQUALS for us, except for cases where the casts in LONGS_EQUAL were actually hiding a bug:

#define CHECK_EQUAL_LOCATION(expected, actual, text, file, line)\
  do { \
     auto expectedLocal = (expected); \
     auto actualLocal = (actual); \
     if (expectedLocal != actualLocal) { \
        UtestShell::getCurrent()->assertEquals(true, StringFrom(expectedLocal).asCharString(), StringFrom(actualLocal).asCharString(), text, file, line); \
     } else { \
       UtestShell::getCurrent()->assertLongsEqual((long)0, (long)0, NULLPTR, file, line); \
     } \
  } while(0)

Edit: To be clear, I am not suggesting changing or unconditionally removing LONGS_EQUAL; just allowing us (and anyone else for whom it is valuable) to conditionally remove them from the API (similar to the LONGLONG versions except it would be opt-out rather than opt-in to maintain backward compatibility).

[basvodde]

Hi @CiderMan

Thanks for the suggestions.

My initial reaction is to decline these two suggestions.

1. I'd rather not have all features of CppUTest under an #if so that they can be switched on and off. We've always tried to have as little of them as possible... and look how many we already have :) A simple way around that is to use a helper around the CppUTest header file in your project that #undefs it or redefines it to something else.

2. So far, we've avoided "auto" in the CppUTest main code... and I'd like to keep it that way. In general, we try to use limited and old C++ so that the code is maximum portable to old systems (often embedded systems or embedded compilers). However, you can use the current CHECK_EQUAL, right?

[CiderMan]

Hi @basvodde,

Regarding your point 1, yes, this was already my fall-back plan. I just prefer to push features into the upstream package, if I can, as I assume that some others may also find it useful.

Regarding point 2, no, CHECK_EQUAL in its current form is not suitable as a drop-in replacement as, unlike LONGS_EQUAL, it evaluates its arguments multiple times. Some of the uses of LONGS_EQUAL include calls to non-pure functions so it would require manual fettling to the code to allow CHECK_EQUAL to be used. The revised version does not evaluate it arguments multiple times, so is a drop-in replacement. Of course, the wrapper header that you suggest could redefine CHECK_EQUAL but it feels that we are moving away from CppUTest at that point, so I would need to consider whether that is the right thing to do...

Cheers,

S.

[thetic]

We might update the guidance at http://cpputest.github.io/manual.html#assertions to include the following pattern:

long result = mock_returning_11();
CHECK_EQUAL(10, result);

auto could be substituted for long where supported.

[basvodde]

@thetic Yes, I think that is a good idea.

[basvodde]

@CiderMan Yes, I understand point (2).

I still do not want an auto in the macro (sorry!). Another thing is... if the code needs to be platform independent then perhaps it is also an issue with using long directly in the codebase? Wouldn't you have this problem also within the production code itself?

I wouldn't consider using custom headers around CppUTest to be moving away though. In fact, I would encourage everyone to make custom asserts for your own codebase. Very often making custom asserts is one of the best ways to make your tests more readable and it is surprisingly easy. It is also why CppUTest has _TEXT versions of all the macros because they are excellent when making custom asserts/

[CiderMan]

@basvodde (& @thetic),

Wouldn't you have this problem also within the production code itself?

We use fixed width types in our target code - for precisely the reason that we need to be portable across several different platforms... although we only run unit-tests on Linux and Windows. The issue is that some unit-tests, in the absence of any fixed width assertions, have used LONGS_EQUAL but done so incorrectly for 64-bit values (which only works under Linux, not Windows). We have issued advice not to use LONGS_EQUAL for this reason but advice typically only results in 99% of the developers getting it right 99% of the time! Hence, we would like to both get rid of whole class of errors that it enables and provide a drop in replacement to replace the > 1,000 instances without needing to hand edit.

As you suggested, I will stop people from including the CppUTest headers directly and add an abstraction header that applies the two changes. Thanks both for your time in discussing this.