fix: hide multiple useless warnings for default build#6773
fix: hide multiple useless warnings for default build#6773PastaPastaPasta merged 2 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe patch reorganizes the suppression of specific GCC warning flags in the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between f593fb6b83f323e919f59bee4fc2a9415e874b60 and eacd3df. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
configure.ac (1)
454-462:-Warray-boundsnow fully suppressed, but flag string still appended when already present.Moving this block outside the
enable_werrorguard meets the PR goal (silencing the warning for non--Werrorbuilds) and switches to-Wno-array-bounds, which is correct.Minor nit: if
NOWARN_CXXFLAGSalready contains the same token (e.g. from a previous configure run with cache), you end up with duplicates. Harmless, but you can avoid it:- NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-array-bounds" + case " $NOWARN_CXXFLAGS " in + *" -Wno-array-bounds "*) ;; + *) NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-array-bounds" ;; + esac
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 54c5f63 and 69ccb409797e35fcd5c0549e69214c0f9aa35471.
📒 Files selected for processing (1)
configure.ac(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
configure.ac (2)
Learnt from: kwvg
PR: #6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The GMP_GCC_ARM_UMODSI macro checks only the compiler version, and GMP_GCC_MIPS_O32 relies on the -mabi=32 flag. Therefore, $LDFLAGS is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
configure.ac (2)
Learnt from: kwvg
PR: #6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The GMP_GCC_ARM_UMODSI macro checks only the compiler version, and GMP_GCC_MIPS_O32 relies on the -mabi=32 flag. Therefore, $LDFLAGS is irrelevant to these tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build container / Build container
- GitHub Check: Build slim container / Build container
🔇 Additional comments (4)
configure.ac (4)
446-453: Missing‐fifix removes build-breaking syntax error — good catch.The added
fiproperly terminates theif test "$enable_werror" = "yes"block and eliminates the Autoconf parse error that slipped through earlier.
No further action needed.
463-468:-Wdangling-referencestill only demoted, not suppressed. Intentional?Unlike the neighbouring changes, this block keeps
-Wno-error=dangling-reference, which still prints the warnings for default builds (they’re just not fatal).
If the objective is “hide the warning”, switch to-Wno-dangling-referencefor consistency.
470-478: Inconsistent pattern with-Wattributes.Same observation as above: you demote to non-error but do not silence. Confirm whether the remaining noise is acceptable; if not, use
-Wno-attributes.
490-502: Good: problematic-Wstringop-*warnings now fully masked for all builds.Switching to
-Wno-stringop-overread/overflowoutside theenable_werrorblock cleanly removes the spam while still testing compiler support throughAX_CHECK_COMPILE_FLAG.No issues spotted.
configure.ac
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
-Wcpp treatment differs from newly added flags.
-Wno-error=cpp leaves the warning visible in normal builds. Re-evaluate if you meant to suppress it entirely as done for -Wstringop-*.
🤖 Prompt for AI Agents
In configure.ac around lines 480 to 488, the -Wcpp flag is currently set with
-Wno-error=cpp, which suppresses treating it as an error but still shows the
warning. To align with the treatment of other flags like -Wstringop-*, change
this to fully suppress the warning by using -Wno-cpp instead of -Wno-error=cpp.
Update the NOWARN_CXXFLAGS assignment accordingly to completely silence the
-Wcpp warning in normal builds.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
Makes sense
utACK 69ccb409797e35fcd5c0549e69214c0f9aa35471
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
Built fine for me 👍
69ccb40979
|
We discussed changes on a call that knst said he wanted to implement |
knst
force-pushed
the
fix-warnings-default-build
branch
from
69ccb40 to
a76e843
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-warnings-default-build
branch
from
a76e843 to
f593fb6
Compare
|
CI failure is unrelated |
They are hidden because they are irrelevant
knst
force-pushed
the
fix-warnings-default-build
branch
from
f593fb6 to
eacd3df
Compare
|
rebase looks wrong: 3d4a6a1?w=1 vs 82d73aa?w=1 |
It's not just rebase, but extra changes.
As discussed with pasta, |
4 participants
Issue being fixed or feature implemented
Default build produces hundreds of warnings. These warnings are disabled for
--enable-werror, but they all are show for regular builds.What was done?
Hide useless warnings for default build too. They have no use.
How Has This Been Tested?
Configure with --enable-werror and without it:
Works fine now!
Breaking Changes
N/A
Checklist: