eranpeer / FakeIt

C++ mocking made easy. A simple yet very expressive, headers only library for c++ mocking.
MIT License
1.22k stars 170 forks source link

Fix colliding mocking context ids #296

Closed otrempe closed 1 month ago

otrempe commented 1 year ago

Fixes https://github.com/eranpeer/FakeIt/issues/294

otrempe commented 1 year ago

Ok... I need to rework constExprHash so no C++14 features are used

FranckRJ commented 1 year ago

I haven't tried to see if it give the same result, but maybe something like this ?

constexpr size_t hash(const char* str, size_t val = 14695981039346656037ULL)
{
    return *str == '\0'
        ? val
        : hash(++str, val ^ static_cast<size_t>(*str) * 1099511628211ULL);
}

https://godbolt.org/z/7vqYqPTPh

EDIT: It probably won't give the same result because the order of evaluation of the parameters is unspecified, maybe ++str will evaluate first, maybe *str. To fix that we can simply write str + 1 instead of ++str, so the variable won't change its value.

EDIT 2: And it seems that the multiply operator take precedence over the bitwise XOR operator, yet again something that makes it non conforment (but easily fixable with parenthesis).

coveralls commented 1 year ago

Coverage Status

Coverage decreased (-4.0e-05%) to 99.925% when pulling 2230a37a81a065cb2c0caf73cfb84ce1d5dc8ff6 on otrempe:Fix-CollidingMockingContextIds into 78ca536e6b32f11e2883d474719a447915e40005 on eranpeer:master.

FranckRJ commented 1 year ago

You need to add the new test files to https://github.com/eranpeer/FakeIt/blob/master/build/sources.mk as well, otherwise they won't run for GCC / Clang.

otrempe commented 1 year ago

Coverage decreased by 4.0e-05% :open_mouth: :laughing:

The Visual Studio build yields lots of warning C4307: '*': integral constant overflow However, Microsoft stopped issuing this warning starting with MSVC v19.23.

otrempe commented 1 year ago

I guess my .h/.cpp helper files need to be added to the sources.mk as well

FranckRJ commented 1 year ago

You're right, but only the .cpp.

FranckRJ commented 1 year ago

Thanks for the fix, I'll try to look more into it next week, I'll do some benchmark because I'm curious if it affect compilation time or not.

FranckRJ commented 1 year ago

I didn't see any measurable difference in compilation time with the fix for the FakeIt unit tests with GCC. I guess it's because our test files are small and the includes take a lot of time to compile, maybe with a test file that is a lot bigger (and pre-compiled header ?) we could notice a difference, or maybe the overhead is so insignificant that we wouldn't notice anything anyway. Either way it looks good to me.

malcolmdavey commented 1 year ago

If using precompiled headers (e.g. with VS) it might make the difference more noticeable (if fakeit.h is added to that).

FranckRJ commented 1 month ago

Superseded by #336. I went with a different approach that I explained in the PR.