-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Reduce re-enabled diagnostics #6636
base: master
Are you sure you want to change the base?
Conversation
8306d39
to
2d05e40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gtest WalletTests.FindUnspentSproutNotes
is failing. Otherwise utACK with comments.
I opened sellout#3 onto this branch. |
b2184e7
to
3a4884b
Compare
3a4884b
to
9b4032d
Compare
@@ -2324,7 +2324,7 @@ void CNode::AskFor(const CInv& inv) | |||
|
|||
void CNode::BeginMessage(const char* pszCommand) EXCLUSIVE_LOCK_FUNCTION(cs_vSend) | |||
{ | |||
ENTER_CRITICAL_SECTION(cs_vSend); | |||
ENTER_CRITICAL_SECTION(cs_vSend) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there were more uses of {ENTER,LEAVE}_CRITICAL_SECTION
then I'd suggest altering the macros to avoid the warning, but it turns out there are only 8 uses of either macro in the whole codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited the macros in other places where it made sense (e.g., 52f4105#diff-b1924661640b70276005001174b3b3640f02be7232bb8d9a1b9518dde32f8055R189). But here (and for ADD_SERIALIZE_METHODS
in #6641) the structure of the macro made it non-obvious how I could make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backporting the removal of PostCommand
/OnPostCommand
in bitcoin/bitcoin#9575 instead of fixing it is blocking.
b0dfbbd
to
0e879ef
Compare
Some of the sub-warnings are still disabled.
Our only change is to rename a var from `requires` (which is a C++20 keyword) to `dependencies`.
Most re-enabled diagnostics have been removed. The remaining ones are tied to diagnostics that require deeper changes. This makes the parent diagnostics explicit so it’s easier to identified when the remaining re-enabled flags should be removed from the list.
0e879ef
to
1234ebe
Compare
I will take over this PR. |
Many Clang diagnostic flags both have their own warnings as well as control
other subflags. This PR addresses the warnings that are reported directly by
these mixed flags, reducing the number of sub-flag diagnostics we need to
re-enable.
Since they control other subflags, enabling the superflag often means disabling
other subflags that also trigger warnings.