refactor: add a long list of warnings and fix issues associated with them #5598
Conversation
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
Beside particilar comments I think it is better to sort list of -Werror warnings; at least our own (not from bitcoin's).
Bitcoin's I think better to keep in same order as they do.
configure.ac
Outdated
There was a problem hiding this comment.
nit: isn't trigraphs are removed since C++17?
I think it's quite useless check then.
src/core_write.cpp
Outdated
There was a problem hiding this comment.
Check this one,isn't it doing the same? https://en.cppreference.com/w/cpp/types/is_unsigned
| static_assert(std::numeric_limits<decltype(opcode)>::min() == 0); | |
| static_assert(std::is_unsigned<decltype(opcode)>::value); |
There was a problem hiding this comment.
Like, yes it is, but I wanted a 1 for 1 for the runtime check just done at compile time; they will evaluate the same for all normal integer types; idk 🤷♂️
There was a problem hiding this comment.
It's constexpr since C++17, so, should work for any type:
template< class T >
inline constexpr bool is_unsigned_v = is_unsigned<T>::value;
which elsee runtime checks do you mean?
There was a problem hiding this comment.
Yeah, for sure it's constexpr, but the runtime check I'm removing is specifically 0 <= opcode and the most intuitive way imo to say that's always true is to say the min is 0; but honestly using the unsigned is probably appropriate too
There was a problem hiding this comment.
Actually interesting it seems that static_assert(std::is_unsigned_v<decltype(opcode)>); fails as opcodetype is an enum and I guess enums aren't considered unsigned?
src/script/interpreter.cpp
Outdated
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
more-compiler-warnings
branch
from
33f7a5c to
f9d3d59
Compare
PastaPastaPasta
force-pushed
the
more-compiler-warnings
branch
from
f9d3d59 to
247d5ba
Compare
|
This pull request has conflicts, please rebase. |
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: N/A - This is a Dash-specific compiler warning enhancement, not a Bitcoin backport Issues found:
Since this is not a Bitcoin backport, the standard backport verification process does not apply. However, the PR has merge conflicts that prevent it from being merged cleanly. Recommendation: The PR author needs to:
Please address these issues and re-run verification after rebasing. |
4 participants
Issue being fixed or feature implemented
Enable additional -Werror compiler flags; tested with clang-17; interested to see if CI is happy
What was done?
Enabled warnings and fixed issues; added some static_asserts for checks which were not needed due to the types used.
How Has This Been Tested?
Building
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.