Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix colliding mocking context ids #296

Closed
wants to merge 5 commits into from

Conversation

otrempe
Copy link
Contributor

@otrempe otrempe commented Nov 9, 2022

Fixes #294

@otrempe
Copy link
Contributor Author

otrempe commented Nov 9, 2022

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

@FranckRJ
Copy link
Collaborator

FranckRJ commented Nov 9, 2022

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
Copy link

coveralls commented Nov 9, 2022

Coverage Status

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

@FranckRJ
Copy link
Collaborator

FranckRJ commented Nov 9, 2022

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
Copy link
Contributor Author

otrempe commented Nov 9, 2022

Coverage decreased by 4.0e-05% 😮 😆

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

@otrempe
Copy link
Contributor Author

otrempe commented Nov 9, 2022

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

@FranckRJ
Copy link
Collaborator

FranckRJ commented Nov 9, 2022

You're right, but only the .cpp.

@FranckRJ
Copy link
Collaborator

FranckRJ commented Nov 9, 2022

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
Copy link
Collaborator

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
Copy link
Contributor

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

@FranckRJ FranckRJ added this to the 2.4.0 milestone Dec 2, 2022
@FranckRJ FranckRJ changed the base branch from master to dev-2.4.0 December 2, 2022 14:43
@FranckRJ FranckRJ modified the milestones: 2.4.0, 2.5.0 Apr 17, 2023
@FranckRJ
Copy link
Collaborator

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

@FranckRJ FranckRJ closed this May 12, 2024
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.

Colliding MockingContext ids when used in multiple translation units because of __COUNTER__
4 participants