Add test/mock support for long long.

[uecasm]

This adds bool and long long/unsigned long long support to CppUMock. (Including tests.)

bool is supported unconditionally on the C++ side and conditionally on the C side (included by default for C11 or where configure detects stdbool.h; excluded by default otherwise). The user can define macros in the project or when running configure to force this on or off as desired.

[unsigned] long long is supported for MSVC or for C++11 or if configure thinks the compiler is fine with it. For the latter case, -Wno-long-long is added to prevent warning spam if pre-C++11. Again, the user can define macros in the project or at configure time to force this on or off as desired.

Tested on 32-bit MSVC 2013 and 64-bit GCC (both default and C++11). I've tried to make the detection robust for other cases but I welcome any improvements.

Currently it considers bool and int to be incompatible types (except in the special case that C++ can set a bool and non-bool-supporting C can fetch it as an int). Also the APIs disappear entirely if not enabled, rather than defining some fallback, so you'll get compile-time errors if your tests expect long-long support but don't enable it. I'm open to changing these (or other things) if alternatives are preferred, or splitting this to separate PRs for bool and longlong.

[basvodde]

Uhm, I won't be doing the "long long" support as I'd like to keep that out to support older compilers.

[uecasm]

That's why it's conditional, so it can be omitted on the older or embedded compilers.

[basvodde]

:)

[basvodde]

I'm a bit uncomfortable with all the LONG LONG #defines in the mocking interface. But, I can't think of a better alternative.

[basvodde]

Yup, but this isn't fully working yet because we ought to ship a variant of config.h when it is created. Right now, we don't :(

[uecasm]

config.h does get created if you run configure. For other build environments it will skip that part and go to the C++11 check below, since any C++11 compiler is supposed to include it.

As currently implemented, the HAVE_LONG_LONG_INT check is only really "safe" via configure, as it also adds -Wno-long-long to avoid GCC warning spam if long long is supported by the compiler but you're compiling pre-C++11, when it was made official. I don't know if CMake has an equivalent.

[basvodde]

I'm uncomfortable with all the #ifs (which we so far been avoiding as much as possible in CppUTest). @arstrube what do you think?

@uecasm It seems the travis build with the old Makefile fails though on a segmentation fault.

[arstrube]

Well, I think it's no worse then HAVE_FORK ;-). It would be really helpful for people working with Windows to have long long support. We are still not using long long internally.

So I am for it.

[arstrube]

Crash seems to happen in MockSupport_c, but I don't see an obvious reason.

[uecasm]

I believe the crash is due to the auto-detection resulting in different values for the CPPUTEST_USE_LONG_LONG in the C and C++ compilers, which results in different lengths for the vtable structures and thus calling the wrong function at runtime. (It wouldn't be an issue if the tables were populated by the C compiler, but that's not the case at present.)

(I had a similar crash once before revising the auto-detection logic -- and the crash did not occur if I ran either of CPPFLAGS=-DCPPUTEST_USE_LONG_LONG configure or CPPFLAGS=-DCPPUTEST_NO_LONG_LONG configure.)

Options I've considered (but not decided on) to resolve this are:

1. Making the vtables unconditional, and installing stubs where needed -- this should avoid the crash but gets a bit messy as the stub signatures can't include types that aren't permitted by the compiler options, and could still generate unexpected failure (though not a crash) if the calls are actually made when the settings are mismatched, since the stub will have to do nothing or return an empty value.
2. Leaving the vtables as they are but stripping out the auto-detection code, such that the library configurer/builder must explicitly enable stdbool.h or long long support if they want them. Since this will be a project-level define (or configure setting) it should always be the same for both C and C++ that way. (Has the added bonus of being able to permit stdbool in C99 this way, at the cost of the feature being absent by default instead of intelligently detected. Also unless you add some new profiles that enable this, it will go untested by CI.)

On a related note, the patch as it stands only includes mods to the autoconf script to introduce new options, since I'm not familiar with how to do this in CMake. If you can suggest where to look I can try adding this too, though I'm not sure if I can test it.

[basvodde]

Yes, the crash helped me understand why I cannot accept the PR as it is and where my discomfort came from. So far, in CppUTest, we've avoided #if in header and especially in classes. Because keeping them in the classes can lead to a mismatch between the lib and the header files that cause painful crashes that will be hard to find for more people.

The alternative to add stubs for when long long isn't there is even more ugly as that will lead to an #if and an #else in the header, and a difference in parameters.

@arstrube This makes it very different from HAVE_FORK.

@uecasm I do feel bad about this, it is a lot of work that has gone into it. I think in the Mock it won't be possible to fix, except via the WithType structure and create a long long variable for that. For the asserts, I think it could be possible to get that done without adding a method to the class, correct?

[arstrube]

I have to agree with @basvodde, after all. It is a pity; I wasn't aware that long long support goes a lot deeper than one would tend to think.... Is the bool different?

[uecasm]

It's a side effect of how the C++/C interface is constructed -- the declarations are compiled as C but the actual implementation is compiled as C++. As such it's very fragile to mismatches in the compiler settings. As I said though it'd be very easy to fix by making it manually selected rather than an auto-detection, or putting in a placeholder to ensure the vtables are the same size -- the problem is that different mixes of C and C++ compilers and libraries end up auto-detecting differently.

The only place in the code where this issue occurs is in MockSupport_c, due to the mix of compilers. It doesn't affect any other part of the library.

[basvodde]

I'm afraid it isn't. To simulate this in pure C++, check this.

Header vtable.h

class A
{
  public:

#ifndef NOFUNC
    virtual void func();
#endif
    virtual void anotherfunc();
};

extern A* createA();

Then, the implementation:

#include "vtable.h"
#include "stdio.h"

A* createA()
{
  return new A;
}

void A::func()
{
  printf("func");
}

void A::anotherfunc()
{
  printf("anotherfunc");
}

And the main.cpp:

#define NOFUNC
#include "vtable.h"

int main()
{
  A* a = createA();
  a->anotherfunc();
  return 1;
}

---

This will print "func" which is the wrong output. I can also switch them around and then it would actually cause a crash. It is pure C++ and it is because of the offset changes in the header due to the #ifdef.

The problem becomes even more serious if you consider that CppUTest can be installed as library, and thus if you would "switch on" the LONG_LONG support via preprocessor #defines, it would crash and people would have a very hard time finding that bu

[Andne]

Could the function always be there but make the first argument a typedef that could be defined as either long long or some other type so that it always exists? Then you could move the actual detection logic into the function, so if long long isn't supported, print a failure error saying so and if it is supported run the correct logic. I think that would solve the vtable issues.

There's a minor issue where the arguments might not line up correctly, but I don't know if that would cause any problems since you wouldn't actually be using the argument if CppUTest was compiled without and there's no reason for someone to expect the long long functions to work if they are compiling without support even if the library was compiled with support.

[uecasm]

@Andne: That's what I meant by having stubs. It'd work fine except in the single case where you had a pre-built CppUTest with different options than when building the application/test code itself. (It's a little more complicated than you suggest above, due to the overloaded functions, but it's still doable.)

@basvodde: People ought to be used to the idea of having to have matching API/ABI-changing options, though -- it's an issue with any other library too (just look at how many different flavours the standard libraries come in) -- and even just using headers from a different version of the library than you're linking to will do it as well. Having said that, there are some techniques that can be used (which are themselves ABI breaking but keep the same API so they're mostly invisible to users) that can be used to generate linker errors rather than crashes if the options are different between library and application. In fact on Windows+MSVC there's an easy way to make it so that the options themselves select between multiple different pre-built library binaries; I don't think you can do the same on Linux/GCC though, you just have to rely on the user getting it right or generating the link errors. Doing this would be fairly straightforward, although it would require changes to all the files.

Putting all that aside for now, though, another simple alternative would be to enable both of these by default -- most modern compilers support both of these types anyway (albeit with warnings in some cases since CppUTest enables all the paranoid/extension-use warnings). The defines would be left in place to still allow users of more esoteric/embedded compilers to disable them if need be -- and folks on those platforms are less likely to use pre-built binaries in the first place, so they're less likely to run into any ABI issues.

How does that sound?

[Andne]

Guess I didn't look far enough to see the changes on the mocking classes. A lot of the functions should work fine since there aren't any overloads, it's the functions with overloads that are concerning. As a side note to that, I've learned that overloading virtual functions in a class hierarchy can have some undesired side effects, especially if derived classes don't override all versions of the function. Maybe it would be good to make the specifically named functions the virtual functions and make the overloaded
"convenience" functions non-virtual and just have them call the virtual functions? I think that would make it possible to still keep the functions despite the build configuration and just change the behavior with changing the interface.

I think that as long as you can keep the interface the same, building with different long long support options shouldn't matter. Also, there are times when I've explicitly disabled language features when building for the host system because I won't have them on target. I shouldn't have to recompile a library (especially not one that may be system-installed) just because I'm not using all of it.

[basvodde]

@uecasm There is a BIG difference between a linker error (ok-ish) and completely the wrong behavior. Switching on/off functionality in this way is just not an option. In fact, it is one of the reasons why gtest it recommended to be in your build rather than build as library.

I'm going to be strict on this and I will not allow #if in the header when it would change the memory layout of the class. Also, the #if everywhere is insanely ugly and when starting with one, there is no return.

The options I see now are:

-> Make the CHECK, but without adding a new assert to the class. This is possible as you can use the old asserts
-> For Mock, you can use the WithType("long long") and implement it that way.
-> If the typedef option from Andy doesn't look ugly, that is possible as well. So that means there would be cpputest_long_long type or so, and no optional methods in the header.

[uecasm]

@basvodde I feel like you didn't actually understand my previous post. I was agreeing that misbehaviour is bad, and suggesting making it generate linker errors instead.

Or if the autodetection is removed and the option is enabled by default, then nobody will ever even notice anything except in the rare case where an old/esoteric compiler doesn't support it, in which case you'll get a compiler error until you set a simple project-wide define to disable the code and you'll never see a linker error OR a misbehaviour. And if need be, we could still put in the support for making linker errors in case you somehow mix up the options between library and app.

The other suggested solutions don't really address anything.

[Andne]

@uecasm May I suggest that you split the bool logic out into a separate pull request and we continue to discuss long long? @basvodde has said that he's willing to include bool, so let's get that in.

There is a case where I may have a copy of CppUTest shared across multiple projects. If those projects are using different target hardware - one on AVR, one on C2000, and one on ARM let's say - I may be building with different compiler options for each project. Likely I am still building all projects as x86 as well so that I can run unit tests quickly and easily. In order to keep myself from doing things that won't work on target when building for host, I like to change compiler options for all configurations to disallow certain features. long long on AVR is probably a bad idea (if it even works), long long on ARM likely doesn't matter, so I may have that option set differently between the two projects. However, I likely want to use the same copy of CppUTest compiled for the host system across both projects. As long as the vtable gets modified based on a project-specific option (which having #if/#ifdef virtual functions will do), this will probably cause a crash.

BTW, I do change compiler options between CppUTest and my project today in one case. I have a target that doesn't like exceptions, so the easy way to make sure they stayed out was to always disable exceptions in my project (-fno-exceptions -fno-rtti). Since my target and host both use GCC, I liked having the options identical to help keep the behavior the same. CppUTest likes to use exceptions in order to correctly handle test failures, so I built that with exceptions. Since all of the exception logic is in code files only, this works properly for me.

[uecasm]

I suppose it depends what you mean by project-specific. Typically you'd have one copy of the CppUTest source installed in one place. The setting of USE/NO_LONG_LONG is baked into that copy of the source at configure time. Any application projects that use it inherit this setting from the CppUTest source they include -- it can't be changed at the application level.

If you want different values for different applications, then you have to build separate CppUTest configurations. This is no different than if you want a C++11 build and a not-C++11 build, though. (I'm not sure how CMake works, but with autoconf, it's just a matter of running configure in a separate folder to create a build with a separate configuration.)

The same applies if you're using MSVC -- the CppUTest project file or manually created config.h file defines the setting, and the application has to agree with it (which is easiest if it's done via a config.h file rather than a project setting, since then it happens for free).

[basvodde]

I understand. The problem is that when a user does change the defines on application level, it will crash
or have unpredictable behavior without giving a proper (linker) error. I've personally had to debug this mistake several times and it is a bitch.

So, in CppUTest, we decided that we don't want to do that. That is nice because we don't want to have #if littering the code all the time, so these two design goals go hand in hand. So, you can chose an alternative way of dealing with long long, but the #if in the header (in the class definition) won't be accepted.

For the mocking, it is either using "WithType" or using a typedef.
For the CHECKs, you can use existing checks, which sucks a bit as the macro might be bigger than previous ones, but it is better than an #if in the UtestShell.

[Andne]

@uecasm The application doesn't have to agree with a lot of settings, as long as the library was built with a superset of the valid configurations. While some settings may need to agree, there's little reason to force others to when it's not required.

[arstrube]

What other platforms besides 64 bit Windows are there that mandate the use of long long? Could it be an option to offer this feature specifically for Windows? As far as I know, long long is needed only for platforms that have 64 bit processers with 32 bit longs (which are a pain to deal with already).

[uecasm]

@basvodde
If the undefined-behaviour-without-link-error is your main issue with that, then as I said above it's possible to do some things with namespaces which are typically invisible from the PoV of user code but will generate link errors if you try to link app & lib with incompatible settings. I can fairly easily include this if you like, but it does require a (trivial) modification to every header file.

(And this is not a new/weird concept; Boost uses it extensively, for example.)

@Andne
That doesn't make sense to me in this context. The only things the settings do is to define whether specific functionality is available in the test library (and even that is only an option because of compiler quirks). If the app doesn't want to use the functionality, then it just doesn't call the methods -- it doesn't care what the setting state is. If it does want the functionality, then it does call the methods, and there will be a compile error if it calls something that isn't defined according to the option as the app sees it. And if we do the above then you'll get a link error if the option as the app sees it is different from the option as the lib sees it.

Or we can put in stubs (such that the methods are always defined, albeit sometimes with a dummy type, in which case it's similar to the above but in the case where the app thinks the options are enabled and the lib doesn't, you'll get an exception at runtime (or whatever the stub does).

Again, though, this is no different than building the library with C++11 support and the app without, or vice versa, since ABI compatibility is not guaranteed between those two configurations. Ditto for using different CRT/STLs (eg. different VS versions). And there's no protection against that either; the user is just expected to not be silly.

@arstrube
long long is useful whenever you want to manipulate numbers larger than 2^32. Sometimes you want to do that even in a 32-bit environment, in which case it's not Windows-specific. (Personally, I would have preferred to use [u]intNN_t types instead, but they need modern compilers or a polyfill header.)

[basvodde]

@uecasm CppUTest doesn't use namespaces (again by design). And, I'm really going to insist to not have #if in class definitions. Its a slipperly slope when we start doing this. I'm sorry.

The bool doesn't seem to have the same #if problem, so that is fine. Doing the CHECK_LONG_LONG without adding an assert function doesn't seem hard. So we're getting a long way with these solutions.

[uecasm]

@basvodde The bool does have the same problem internally in MockSupport_c, which is what started this whole discussion. It only presents itself if the auto-detection sets the option differently for the C code vs. the C++ code, since the vtables are defined in C but populated in C++, and C/C++ lack named initialisation (which would have avoided the whole mess).

That could be fixed by removing the auto-detection and letting it be manually enabled or disabled (depending which way it's set by default), to ensure it's consistent between the two. Or by just pretending that C never has a bool.

[arstrube]

@uecasm wrote:

This is no different than if you want a C++11 build and a not-C++11 build, though.

Actually, it is quite different. In my experience, whether or not I built CppUTest with C++11 enabled makes no difference to the library, or to the code using it. In fact, I am using this for the IEEE754 floating point flag detection. As long as the plugin is compiled with C++11 enabled, there is no need for tests or the code that is tested to be compiled with C++11 enabled.

[Andne]

@uecasm I think I got project and compiler settings mixed up. Right, anything in the code files has whatever settings were there when the project was built and that can't change. Settings in the header files can change though. Depending on my application project, I may have a need/want to disable long long support in the compiler to have an extra check that it didn't get used accidentally. If the vtable of a class in the header has functions that are conditionally added based on long long support, this changes how may application see the vtable layout and will likely will result in the wrong function being called when I call anything that comes later on in the vtable. If those functions are always there and just unused, then the vtable never changes and so that crash becomes a non-issue. If CppUTest got built without long long and then a project tries to use long long the stub methods should be able to provide a runtime error that the library doesn't support that feature.

[uecasm]

@arstrube Have you tried using StringFrom(const std::string&) in a mixed environment? There's no guarantee that this type has the same layout in C++03 vs. C++11 cases (although as long as you're using the same version of the STL then you might get lucky if the author has chosen to make them compatible anyway). The same issue could occur with any other STL types, although CppUTest mostly avoids using those so it's not as affected as other software can be.

@basvodde I'm curious why you're avoiding namespaces. Are you trying to support compilers that don't support namespaces? I don't see how they can call themselves C++ compilers in that case. And the way this would have been done would still put all the symbols in the global namespace, so user code wouldn't have to change.

But ok, I'll modify the patches as follows:

1. Add a dummy typedef for the longlong-disabled case (and the bool-disabled case in C).
2. Remove (almost) all of the #if from the headers so that the classes have (mostly) the same interface, apart from the typedef as above. Instead use stubs that will throw at runtime if the app manages to call them despite not being supported.
3. Remove the auto-detection and disable C-bool and longlong by default (so they're opt-in).
4. Not add any namespaces for protection via link errors.

Is everyone happy with those modifications? (Note though that disabling these by default means that the CI won't be testing it, until you add more profiles.)

[coveralls]

Coverage decreased (-0.3%) to 99.56% when pulling 4ba1baf on uecasm:bool-longlong into 98e0736 on cpputest:master.

[arstrube]

Yay. Getting there!

[arstrube]

While the percentage change seems small now, some rather important methods are not covered :-(

[arstrube]

@uecasm actually, the coverage problem here seems to be a general one... just like to hear your opinion on this... in my experience, long long is specifically needed in Windows environments, because on Windows 64 bit environments, a long is still only 32 bits. On 64 bit 'nix environments, long is 64 bit anyway.

We collect coverage under Linux, but we need to cover some functionality, that is essentially mostly for Windows. On the other hand, long long should work the same under Linux, so couldn't we simply enable long long for the coverage build, and disable it for the rest?

Just wondering.

[uecasm]

64-bit values are also useful when compiling for 32-bit Linux, so it's not unique to Windows. Also, if you do happen to be compiling for 64-bit Linux, then this will (typically, though it depends on the compiler) support 128-bit values, so I think it has reasonable value across the board.

Regarding coverage, this doesn't reduce the coverage on any existing methods, it's just some of the new ones that aren't fully covered when longlongs are disabled. I'm not sure how to run the coverage locally so I'm just covering what I can and see what happens after I push. 😁

I'm imagining that additional CI profiles will be added to build both with and without longlong support, including coverage for both. That's up to @basvodde though, of course. The CI is already building at least six times per profile anyway for some reason, so I suppose it could be added at that level rather than in the CI itself.

MockExpectedCallComposite, I can fix up. assertLongLongsEqual, I thought I already had; I must have missed something (it's definitely covered if longlongs are enabled though).

The single-line uncovered things in withActualLongLongIntParameters_c and MockCheckedActualCall are covered when long-longs are enabled but not when disabled as the prior line raises an expected failure. I doubt there's much that can be done for this except telling coverage to ignore that line.

[arstrube]

I see. Well, if it is useful across the board, all the more reason to get it tested on all systems.

Actually, I wasn't thinking of adding anything to the CI (besides, that wouldn't help with the coverage results), I was more thinking of configuring the Coveralls build such that it will be tested.

It's easy to test on your fork (not locally).

1. Make sure Travis is run on new commits on your fork
2. Create a Coveralls account and connect it to your fork
3. Configure it such hat it will run when you push to your branch - OR -
4. Create a pull request against your own fork.

To run coverage locally, the most convenient way is to use Lcov. At a minimum, you need to do:

$ ../configure --enable-coverage CFLAGS="-O0 <whateveryouwant>\
   CXXFLAGS="-O0 <whateveryouwant>

Then you need to run Lcov after building and running the tests. If you'd like to explore that route, I can share the cmds I use to do this (don't know by heart).

[arstrube]

P.S. I often run experiments on a local branch that I graft onto the 'real' pull request only when the checks have passed to my satisfaction on my fork. I give my local branch a very short and non-descriptive name for convenience ;-). By 'local' I mean a branch that I push only to my own fork, and that is not associated to a PR.

[uecasm]

I think this one should cover everything but the single-line-after-fail cases. They're covered if longlongs are enabled, but I think the only way to avoid them going red when disabled is to make coverage ignore them, and I didn't want to do that at least until you both can look at them and agree that they're harmless. 😀

[arstrube]

For single line after fail cases, we use /* LCOV_EXCL_LINE */ - in other similar places, too. As far as I know, Coveralls respects that.

[uecasm]

Yes, I know. I've already added it in the tests that are designed to fail (that test the failure reporting), similar to the existing ones. But as I said, I didn't want to add it on the other ones (since they're in real code, not tests) until you've both had a chance to look at the results and agree that they're ok to be ignored. Hopefully (other than the pre-existing platform red spot) they'll be the only red spots in this build, so it should stand out.

[coveralls]

Coverage decreased (-0.07%) to 99.841% when pulling c0daa43 on uecasm:bool-longlong into 98e0736 on cpputest:master.

[arstrube]

Hmm. No, this is NOT the classical return after failure case, which would be something like:

int func() 
{
    // .... stuff
    FAIL ("A failure occurdd");
    return -1; // This is the classical return after failure
}

e.g. whatever you do, this cannot be executed. The code in question can, and should, execute in case of a successful test.

Why isn't it possible to make the test succeed?

[uecasm]

All the tests pass. But that line can never be reached when longlongs are disabled, because attempting to fetch a longlong value in that case triggers an "unsupported feature" mock failure, which throws and aborts the test before it hits that line of code. See MockNamedValue::getLongLongIntValue and TEST(MockExpectedCall, callWithLongLongIntegerParameter).

The line will be reachable when longlongs are enabled.

[arstrube]

Okay, that wasn't obvious. What stops us from enabling longlongs at least for the coverage build?

[uecasm]

I'm not sure what needs to be changed to make that happen. And it still seems worthwhile to test (and possibly coverage?) both cases.

[arstrube]

Yes, the other case will be tested everywhere else, right? Since the coverage build is done with Cmake, there needs to be the #define set for this target to enable longlongs. I'm not so familiar with Cmake; perhaps a configuration option needs to be created for it (which the Cmake folks would need in any case), or perhaps (like I outlined for Automake above) a variable can be set via commandline.

[arstrube]

An addition to this line in travis_ci_build.sh, I would say:

67    cmake .. -DCMAKE_BUILD_TYPE=$CMAKE_BUILD_TYPE -DCOVERAGE=ON

[coveralls]

Coverage decreased (-0.2%) to 99.737% when pulling 0617b56 on uecasm:bool-longlong into 98e0736 on cpputest:master.

[basvodde]

As next step, can we separate out the long long CppUTest and the mocking support?

[uecasm]

Separate commits ok, or do you want a separate PR?

[basvodde]

I mean a separate PR.

Sorry for the additional trouble, but having 48 files changes in a diff makes it really hard for me to see how they are all linked. Usually I review code in a relative short time, which means that large PRs like this tend to never get merged :)

This is why the bool did get merged, as I could much better see what was all going on :) As Dijkstra, blame it on my small head ;P

[uecasm]

Ok, can do. It probably won't be for a couple days though since I'm into the weekend already. 😴

[basvodde]

Likewise :)