Fix Firo Build Warnings#1872
Conversation
|
User rahimi.nv@gmail.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe PR updates CMake warning propagation and dependency packaging, removes the ChangesBuild and dependency configuration
Qt and portability
Spark and wallet behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202dc508e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -1,4 +1,4 @@ | |||
| packages:=boost openssl libevent gmp zlib backtrace tor bls-dash | |||
| packages:=boost openssl libevent gmp zlib backtrace tor bls-dash immer | |||
There was a problem hiding this comment.
Adding immer to depends is not enough after deleting the vendored src/immer tree: the main CMake files never find_package/link an immer target or add the depends prefix include directory, while many production files still reach src/evo/deterministicmns.h:16 and include immer/map.hpp. In a normal clean depends build those headers install under the depends prefix but are not on any target's include path, so node/wallet/Qt compilation fails with the vendored headers gone; please add an immer include/target in CMake or keep the headers vendored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
immer is header only and installed to (custom) root defined by toolchain. So it is okay.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/sparkname_tests.cpp (1)
793-809: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! The non-capturing lambdas are valid in C++20 —
overflowBlocksis aconstvariable of literal type initialized with a constant expression, so reading it is not an odr-use and doesn't require capture.Optional: marking
overflowBlocksasconstexprwould make the compile-time constant intent explicit and unambiguously guarantee the non-capturing lambda is valid across all conforming compilers.Optional: use constexpr for clarity
- const uint32_t overflowBlocks = (uint32_t)INT_MAX + 1u; // minimal case that triggers the bug + constexpr uint32_t overflowBlocks = (uint32_t)INT_MAX + 1u; // minimal case that triggers the bug🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/sparkname_tests.cpp` around lines 793 - 809, Optionally declare overflowBlocks as constexpr in the test case so its compile-time constant intent is explicit and the non-capturing lambdas in the fresh-registration and renewal scenarios remain clearly valid.src/qt/bitcoin.cpp (1)
159-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing Qt5 fallback branches if the project only targets Qt 6.
The
#elsebranches usingQLibraryInfo::location()(and similar Qt5-only blocks throughout this file, e.g., lines 750–754, 901–911) are dead code if the project no longer builds with Qt 5. Based on learnings, Qt5-specific code blocks insrc/qt/are no longer needed in this repository. This can be addressed in a follow-up cleanup.Also applies to: 171-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/bitcoin.cpp` around lines 159 - 163, Remove obsolete Qt5 compatibility branches throughout src/qt/bitcoin.cpp, including the QLibraryInfo::location() fallback near qtTranslatorBase.load and the other Qt5-only preprocessor blocks referenced in the comment. Keep the Qt6 implementations using QLibraryInfo::path() and simplify the surrounding conditional compilation accordingly.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/qt/bitcoin.cpp`:
- Around line 159-163: Remove obsolete Qt5 compatibility branches throughout
src/qt/bitcoin.cpp, including the QLibraryInfo::location() fallback near
qtTranslatorBase.load and the other Qt5-only preprocessor blocks referenced in
the comment. Keep the Qt6 implementations using QLibraryInfo::path() and
simplify the surrounding conditional compilation accordingly.
In `@src/test/sparkname_tests.cpp`:
- Around line 793-809: Optionally declare overflowBlocks as constexpr in the
test case so its compile-time constant intent is explicit and the non-capturing
lambdas in the fresh-registration and renewal scenarios remain clearly valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9d953ca-0d07-4434-be9f-890f4298ce03
📒 Files selected for processing (102)
CMakeLists.txtcmake/module/WarnAboutGlobalProperties.cmakecmake/utilities.cmakedepends/packages/immer.mkdepends/packages/packages.mksrc/CMakeLists.txtsrc/bench/CMakeLists.txtsrc/crypto/CMakeLists.txtsrc/crypto/progpow/CMakeLists.txtsrc/immer/algorithm.hppsrc/immer/array.hppsrc/immer/array_transient.hppsrc/immer/atom.hppsrc/immer/box.hppsrc/immer/config.hppsrc/immer/detail/arrays/no_capacity.hppsrc/immer/detail/arrays/node.hppsrc/immer/detail/arrays/with_capacity.hppsrc/immer/detail/combine_standard_layout.hppsrc/immer/detail/hamts/bits.hppsrc/immer/detail/hamts/champ.hppsrc/immer/detail/hamts/champ_iterator.hppsrc/immer/detail/hamts/node.hppsrc/immer/detail/iterator_facade.hppsrc/immer/detail/rbts/bits.hppsrc/immer/detail/rbts/node.hppsrc/immer/detail/rbts/operations.hppsrc/immer/detail/rbts/position.hppsrc/immer/detail/rbts/rbtree.hppsrc/immer/detail/rbts/rbtree_iterator.hppsrc/immer/detail/rbts/rrbtree.hppsrc/immer/detail/rbts/rrbtree_iterator.hppsrc/immer/detail/rbts/visitor.hppsrc/immer/detail/ref_count_base.hppsrc/immer/detail/type_traits.hppsrc/immer/detail/util.hppsrc/immer/experimental/detail/dvektor_impl.hppsrc/immer/experimental/dvektor.hppsrc/immer/flex_vector.hppsrc/immer/flex_vector_transient.hppsrc/immer/heap/cpp_heap.hppsrc/immer/heap/debug_size_heap.hppsrc/immer/heap/free_list_heap.hppsrc/immer/heap/free_list_node.hppsrc/immer/heap/gc_heap.hppsrc/immer/heap/heap_policy.hppsrc/immer/heap/identity_heap.hppsrc/immer/heap/malloc_heap.hppsrc/immer/heap/split_heap.hppsrc/immer/heap/tags.hppsrc/immer/heap/thread_local_free_list_heap.hppsrc/immer/heap/unsafe_free_list_heap.hppsrc/immer/heap/with_data.hppsrc/immer/lock/no_lock_policy.hppsrc/immer/lock/spinlock_policy.hppsrc/immer/map.hppsrc/immer/map_transient.hppsrc/immer/memory_policy.hppsrc/immer/refcount/enable_intrusive_ptr.hppsrc/immer/refcount/no_refcount_policy.hppsrc/immer/refcount/refcount_policy.hppsrc/immer/refcount/unsafe_refcount_policy.hppsrc/immer/set.hppsrc/immer/set_transient.hppsrc/immer/table.hppsrc/immer/table_transient.hppsrc/immer/transience/gc_transience_policy.hppsrc/immer/transience/no_transience_policy.hppsrc/immer/vector.hppsrc/immer/vector_transient.hppsrc/net_processing.hsrc/netaddress.hsrc/prevector.hsrc/protocol.cppsrc/protocol.hsrc/qt/CMakeLists.txtsrc/qt/automintdialog.cppsrc/qt/automintdialog.hsrc/qt/bitcoin.cppsrc/qt/guiutil.cppsrc/qt/optionsdialog.cppsrc/qt/test/CMakeLists.txtsrc/qt/test/rpcnestedtests.cppsrc/qt/walletmodel.cppsrc/qt/winshutdownmonitor.cppsrc/rpc/server.cppsrc/secp256k1/cmake/TryAppendCFlags.cmakesrc/spark/sparkwallet.cppsrc/spark/state.cppsrc/sparkname.cppsrc/stacktraces.cppsrc/test/CMakeLists.txtsrc/test/evo_simplifiedmns_tests.cppsrc/test/serialize_tests.cppsrc/test/sparkname_tests.cppsrc/util.cppsrc/validation.cppsrc/validation.hsrc/wallet/CMakeLists.txtsrc/wallet/rpcwallet.cppsrc/wallet/wallet.cppsrc/zmq/CMakeLists.txt
💤 Files with no reviewable changes (65)
- src/immer/detail/hamts/node.hpp
- src/immer/heap/unsafe_free_list_heap.hpp
- src/immer/detail/hamts/champ.hpp
- src/immer/flex_vector_transient.hpp
- src/immer/detail/rbts/rrbtree.hpp
- src/immer/detail/rbts/operations.hpp
- src/immer/heap/identity_heap.hpp
- src/immer/refcount/unsafe_refcount_policy.hpp
- src/immer/heap/free_list_node.hpp
- src/immer/refcount/no_refcount_policy.hpp
- src/immer/heap/malloc_heap.hpp
- src/immer/lock/no_lock_policy.hpp
- src/immer/detail/ref_count_base.hpp
- src/immer/detail/arrays/node.hpp
- src/immer/refcount/refcount_policy.hpp
- src/immer/transience/gc_transience_policy.hpp
- src/immer/atom.hpp
- src/immer/set.hpp
- src/immer/box.hpp
- src/immer/heap/tags.hpp
- src/immer/detail/hamts/champ_iterator.hpp
- src/immer/detail/rbts/rrbtree_iterator.hpp
- src/immer/detail/rbts/visitor.hpp
- src/qt/winshutdownmonitor.cpp
- src/immer/set_transient.hpp
- src/immer/detail/type_traits.hpp
- src/immer/flex_vector.hpp
- src/immer/array_transient.hpp
- src/immer/detail/rbts/rbtree_iterator.hpp
- src/immer/config.hpp
- src/immer/heap/free_list_heap.hpp
- src/immer/heap/cpp_heap.hpp
- src/immer/detail/arrays/with_capacity.hpp
- src/immer/algorithm.hpp
- src/immer/heap/thread_local_free_list_heap.hpp
- src/immer/detail/rbts/bits.hpp
- src/immer/memory_policy.hpp
- src/immer/transience/no_transience_policy.hpp
- src/immer/experimental/detail/dvektor_impl.hpp
- src/immer/heap/with_data.hpp
- src/immer/heap/debug_size_heap.hpp
- src/immer/refcount/enable_intrusive_ptr.hpp
- src/rpc/server.cpp
- src/immer/experimental/dvektor.hpp
- src/immer/vector_transient.hpp
- src/immer/detail/rbts/node.hpp
- src/immer/detail/arrays/no_capacity.hpp
- src/immer/vector.hpp
- src/immer/heap/split_heap.hpp
- src/immer/heap/gc_heap.hpp
- src/immer/detail/hamts/bits.hpp
- src/immer/array.hpp
- src/immer/map_transient.hpp
- src/immer/detail/combine_standard_layout.hpp
- src/validation.cpp
- src/immer/detail/util.hpp
- src/immer/heap/heap_policy.hpp
- src/immer/map.hpp
- src/immer/detail/rbts/rbtree.hpp
- src/wallet/rpcwallet.cpp
- src/immer/table.hpp
- src/immer/detail/rbts/position.hpp
- src/immer/lock/spinlock_policy.hpp
- src/immer/table_transient.hpp
- src/immer/detail/iterator_facade.hpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
depends/builders/darwin.mk (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
-mmacos-version-minfor consistency with the host file.The builder uses
-mmacosx-version-min(the older deprecated alias) whiledepends/hosts/darwin.mkuses-mmacos-version-min(the current preferred form). Both are accepted by Clang and produce identical behavior, but aligning on the newer form avoids confusion and future deprecation noise.♻️ Optional: align flag form with host file
darwin_CC=$(shell xcrun -f clang) -isysroot$(shell xcrun --show-sdk-path) -mmacos-version-min=$(OSX_MIN_VERSION) darwin_CXX:=$(shell xcrun -f clang++) -stdlib=libc++ -isysroot$(shell xcrun --show-sdk-path) -mmacos-version-min=$(OSX_MIN_VERSION)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@depends/builders/darwin.mk` around lines 13 - 14, Replace the deprecated -mmacosx-version-min flag with -mmacos-version-min in the darwin_CC and darwin_CXX definitions, keeping the existing OSX_MIN_VERSION value and all other compiler options unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@depends/builders/darwin.mk`:
- Around line 13-14: Replace the deprecated -mmacosx-version-min flag with
-mmacos-version-min in the darwin_CC and darwin_CXX definitions, keeping the
existing OSX_MIN_VERSION value and all other compiler options unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 804bfe8d-debf-412a-8b17-89db35b5b120
📒 Files selected for processing (7)
depends/builders/darwin.mksrc/llmq/quorums_dkgsessionhandler.hsrc/qt/forms/addressbookpage.uisrc/qt/forms/recover.uisrc/qt/forms/sendcoinsentry.uisrc/test/dbwrapper_tests.cppsrc/test/mtp_tests.cpp
✅ Files skipped from review due to trivial changes (3)
- src/qt/forms/addressbookpage.ui
- src/llmq/quorums_dkgsessionhandler.h
- src/qt/forms/recover.ui
1 participant
PR intention
This branch focuses on reducing Firo build-warning noise without changing runtime behavior or consensus/wallet semantics. It fixes straightforward project-owned warnings, scopes warning flags more carefully, keeps third- party/vendor warnings out of the main project warning surface, and moves immer into the depends system as a proper CMake-installed dependency instead of carrying it under src/. It also improves the CMake configure output by reporting expected global compile definitions/options as status notices while preserving stricter diagnostics for genuinely problematic global build-system usage.