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
C++ unordered_set hash/comparator requirements #1348
Comments
Original Redmine Comment Which OS distribution and GCC? It sounds like you were able to get it working, could you please post the patch that fixes this? |
Original Redmine Comment OSX 10.13.6 and apple's version of clang. I'm also using libc++ instead of libstdc++.
I attached a patch for the first problem, but I don't know how to resolve the second. |
Original Redmine Comment I have two other small patches that fix warnings from clang - one fixes a format error and the other fixes an unused variable error. |
Original Redmine Comment I pushed to git your three patches, thanks. As to the invokable one I wasn't able to get an assertion compiling to show the issue, I'll need to do more research, what is the exact compiler message you get? |
Original Redmine Comment Here's the full messages. From Compiling src/V3Scoreboard.cpp:
From Compiling src/V3Partition.cpp:
|
Original Redmine Comment Ah - found the problem. CmpElems::operator() was declared non const. Attached patch fixes the warning. |
Original Redmine Comment Great. Strange that cppcheck didn't complain about the missing const, it usually finds those. Sounds like you're all set, but if this still gives problems I think there's an alternative fix that might work. Pushed to git towards 4.004. |
Original Redmine Comment Another possible issue I noticed while working on this - it looks like gcc isn't being called with any -std=XXX even though autoconf recognizes that my compiler is gnu++14 capable. Is that the intended behavior? That means by default it uses the custom unordered_set provided with verilator rather than the system one, which might be why no one else noticed the first issue until now. I'm not super familiar with autoconf or the build system, but it looks like everything is compiled with $CPPFLAGS, but only $CXXFLAGS has the standard flag? Also, not sure if it's related, but in configure.ac on line 237 you have "_MY_CXX_CHECK_SET(CFG_CXXFLAGS_STD_OLDEST,-std=std++03)". Should that be '-std=c++03'? At least with clang, -std=std++XX isn't a legal option. |
Original Redmine Comment I fixed the std++03 thing, that was a typo, though the macro it sets isn't presently used. At present these flags are only used for runtime, that should be fixed, but I'll need to do a lot of testing first as these flags seem to break a lot of stuff in glibc. |
Original Redmine Comment In 4.004. |
Author Name: Kevin Kiningham (@kkiningh)
Original Redmine Issue: 1348 from https://www.veripool.org
Both the hash and comparator functors provided to std::unordered_set must be copy constructible per the C++ standard. Otherwise, compiling verilator with libc++ fails with the following errors
"error: static_assert failed "the specified hash does not meet the Hash requirements""
"error: static_assert failed "the specified comparator is required to be copy constructible""
The error goes away if both EqSenTree and HashSenTree are made copyable.
Also, for some reason std::less and std::less are considered non-invokable in SortByValueMap::removeKeyFromOldVal, which causes a warning. I'm not totally sure why this is since it looks like both have a const operator< member function defined.
You can check for this by adding the following static assert to the first line of removeKeyFromOldVal.
static_assert(std::__invokable<const T_KeyCompare &, const T_Key &, const T_Key &>::value, "Not invokable");
(In c++17 you can use std::is_invocable instead of std::__invokable)
The text was updated successfully, but these errors were encountered: