add support for long long integers as native types

[jacob-keller]

When testing code that needs to work on both 64-bit platforms and 32-bit
platforms, the long integer may not guarantee 64bits of width on the
32-bit platform. This makes testing calls which take and return 64bit
values difficult, as the test framework can't handle these natively.

Add support for long long integers, so that test code can natively
handle these larger values.

Signed-off-by: Jacob Keller jacob.e.keller@intel.com

[jacob-keller]

I think more tests is probably a good idea, but wanted to get this up as a pull request to gauge whether there was interest.

[coveralls]

Coverage increased (+0.004%) to 99.878% when pulling 233f864 on jacob-keller:jk-long-long-support into 03fabe0 on cpputest:master.

[basvodde]

@jacob-keller . I appreciate the pull request. So far, all the long long usage is under a precompiler define so that it works also on old systems without a long long. That is why, so far, we've not accepted yet the mocking support for long long because adding #if in a class with virtual methods is dangerous.

Any alternative suggestion is welcome though!

[jgonzalezdr]

Perhaps shouldn't just using cpputest_longlong and cpputest_ulonglong typedefs like in the test macros be enough?

BTW, some additional unit tests are still needed to cover all the new functions.

[jacob-keller]

BTW, some additional unit tests are still needed to cover all the new functions.

Yea I can work on that part, I found some more issues.

I think the typedefs won't work properly because you'd end up masking functions when the type doesn't exist, no?

[basvodde]

Previous attempt to typedef's didn't work.

[jgonzalezdr]

@basvodde : What is the problem with #ifdef'ing virtual functions? CI builds with C++11 could be used to give early error detection if a override is defined on a subclass but not on the parent class, and also if a override is not defined properly with the override keyword, it's just a matter of adding a C++11 compilation with and without long longs to ensure that functions are defined coherently.

[basvodde]

@jgonzalezdr The problem is that when you accidentally include one header without the define, then your code will accidentally call the wrong method due to the different vtables. It leads to potentially really painful bugs. In general, it is best to avoid #ifdefed virtual functions.

[jgonzalezdr]

@basvodde I more or less see the problem. Why not make the inclusion of CppUTestGeneratedConfig.h to be mandatory in CppUTestConfig.h (i.e. remove #ifndef CPPUTEST_USE_OWN_CONFIGURATION)? The install scripts always install CppUTestGeneratedConfig.h, therefore the generated libraries and its associated includes would always be "synchronized". There would be no way (expect unwise tampering by the user) to accidentally compile a test using a configuration different than the one used to compile the library, isn't it?

[jacob-keller]

Is there a good way to determine test coverage locally before pushing? I want to make sure I've added tests to cover everything I've changed since the previous push.

[basvodde]

@jgonzalezdr That won't work, especially because it will cause a different binary and many people (including me) pick up CppUTest via a package manager.

[jgonzalezdr]

@basvodde But package managers install both include files and binary libraries, therefore if CppUTestGeneratedConfig.h is part of the distribution and is mandatory to exist, there should be no problem, isn't it? I mean, I've used some libraries (e.g. wxWidgets) that #ifdef virtual functions and use a config header specific to the generated binary without any problems, and also didn't notice any users having problems. I think I may be missing the point on a use case that I've never been though... 😕

[basvodde]

@jgonzalezdr I'm skeptical and careful as the problems it can lead to are so hard to debug. If the header and the binary are different then, in the debugger, all the function calls will go to another function with a messed up stack. It is not nice. If we can avoid that, that is better. I'm not sure this functionality is worth the potential pain.

[jacob-keller]

I'm skeptical and careful as the problems it can lead to are so hard to debug. If the header and the binary are different then, in the debugger, all the function calls will go to another function with a messed up stack. It is not nice. If we can avoid that, that is better. I'm not sure this functionality is worth the potential pain.

So if I actually need access to mock functions of these types what is the best way?

[basvodde]

I do not know what the best way is. We had a similar pull request before and decided not to merge that due to the #ifdef around the virtual functions.

[jacob-keller]

I do not know what the best way is. We had a similar pull request before and decided not to merge that due to the #ifdef around the virtual functions.

I don't mean "what shoudl I do to make this includable" since I think there's no other solution besides either (a) require that all compilers support longlongs, or (b) use an #if macro.

What I mean is, "what's the best way to mock a function that takes a long long given that the built in types don't work? Do I have to make a comparator?

[basvodde]

@jacob-keller I think a comparator might actually work. You could then also add convenience methods in or out of the class (as non-virtual) and put these in #ifdef!

[jacob-keller]

@jacob-keller I think a comparator might actually work. You could then also add convenience methods in or out of the class (as non-virtual) and put these in #ifdef!

Yea I'm gonna see if I can do this as a sort of built-in comparator.

[jacob-keller]

Yea I'm gonna see if I can do this as a sort of built-in comparator.

I don't see any obvious way to install a default comparator, but we could use a non-virtual .withParameter overload to call .withParameterOfType("type" ...)

So I think this is the best solution forward if we can find a way to get the comparator installed by default.

[uecasm]

I apologise for going dark on #847 for so long, but that solution just works for me and I've been distracted by other things. If there's still interest in the idea (using #if but providing stubs so the vtable stays in sync) I could look at cleaning up and updating the remaining unmerged parts of that PR?

[jgonzalezdr]

I've pushed a PR on @jacob-keller own branch (jacob-keller#1) that avoids the vtables problem by using a intermediate subclass to contain all the #ifdef'd functions.

That way, as we have simple inheritance, if compiling without long long support, the vtable for the base class will be the same and as expected by the compiler.

If both of you like this solution, just accept the PR on Jacob's repo.

[jacob-keller]

Thanks, I'll take a look once I get some more time, probably Monday.

[basvodde]

@jacob-keller The comparator can be installed in the convenience methods that are outside of the class?

[jgonzalezdr]

There is still another way to avoid the vtables problem without making "special" designs or treating long longs in a different ways, just using #ifdefs:

Two functions can be added, one inline and the other non-inline that just detect the current compilation flags (for now with long long support could be enough maybe). The mock framework could receive in some key function (e.g. MockSuppport's mock()) the result of the inline version as a default value, and then it can just compare that both functions return the same values and throw an error if not.

[basvodde]

@jgonzalezdr I'm quite comfortable with the idea of using a comparator. In fact, almost wondering whether we need to to that for all types :)

I don't think we want to add detection. It is best to just avoid #if.

[jgonzalezdr]

@basvodde Using comparators for everything sounds good, at least the implementation would be homogeneous, and CppUTest is already inefficient because it does string comparisons for the type all the time. But if the capability of comparing integers of different sizes has to be maintained, the comparator design will have to be improved such that the "other" type is also passed and the comparator decides if one type can be compared with the other. This may break backward compatibility with already existing user comparators.

[jgonzalezdr]

As per my previous comment, just in case the comparator strategy is not feasible or whatever, I've revamped my previous PR to avoid subclasses (jacob-keller#2), leaving everything much cleaner.

[jacob-keller]

I merged that into my pull request now.

…

On Sun, Mar 4, 2018 at 3:00 PM, Jesús González ***@***.***> wrote: As per my previous comment, just in case the comparator strategy is not feasible or whatever, I've revamped my previous PR to avoid subclasses ( jacob-keller#2 <jacob-keller#2>), leaving everything much cleaner. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1Y3xjcg7grESyqzZvkH9OS96s-T888ks5tbHIigaJpZM4SVmzU> .

[jacob-keller]

Found another miss, the MockSuport() didn't have cpputest_longlong and cpputest_ulonglong functions, added those

[jacob-keller]

Ok, I think I finally got coverage for all the lines, might be missing a couple in the C test files. It's hard to test it manually since I can't get the coverage report working locally.

[basvodde]

Got some failing tests now though.

[jacob-keller]

It looks like this might be a result of LLONG_MAX not being correct on some platforms? Hmm it seems to work on some of the platforms, but not others?

[jacob-keller]

I'll check what changed between versions, see if something obvious makes sense.. The two test cases don't seem like they should fail, and I've been pouring over the MockNamedValue stuff to see if I could spot it, no luck so far.

[jacob-keller]

I think I spotted what the problem was, plus a few other cleanups

[jacob-keller]

It looks like this might be a result of LLONG_MAX not being correct on some platforms? Hmm it seems to work on some of the platforms, but not others?

Woops, I had accidentally used a .longIntValue_ accessor when I should have used .longLongIntValue_, which resulted on breakages on 32bit platforms.

[jacob-keller]

The travis ci failure appears to be a network issue:

Connecting to ftp.openwatcom.org (ftp.openwatcom.org)|70.165.27.117|:80... failed: Connection refused.

[basvodde]

I'll restart the build when it is due to network failure. But both the Appveyor and the Travis build are failing at the moment...

[basvodde]

Direct C includes are not what we want. Shouldn't happen anywhere at the moment, right? Why is this needed?

[jacob-keller]

I think we need this for the definition of LLONG_MAX and ULLONG_MAX? Maybe we don't tho?

[basvodde]

Almost there. Sorry for doing a bit more requests still. Can you try to seperate out all the long long tests in one section so there is only one (or very few) #ifdef in the file?

I wanted to merge as it, but noticed that there is a C include... that is probably not what we want.

[jacob-keller]

Can you try to seperate out all the long long tests in one section so there is only one (or very few) #ifdef in the file?

I will move all of them together into one block for each file, yes.

[jacob-keller]

Ok, I removed the climits header, and I re-ordered some of the tests in MockParameterValue.cpp

I chose not to re-order EVERY file to combine all the tests in one location, because I felt that the tests didn't go well together. Essentially, I think it's better to keep the tests which are similar to regular typed tests near each other, rather than reduce one or two #ifdefs.

If you feel very strongly, I will make that change, but I'd prefer not to.

[basvodde]

Test failure on cygwin Legit?

[jacob-keller]

Hmm.. Itt's failing in TestDefaultCrashMethodInSeparateProcessTest which I don't think I modified?

Hmm

[basvodde]

Restarted and it works. Merging.

Thanks for your patience and contribution!