The global function has issues:
performance-unnecessary-copy-initialization clang-tidy isn’t run on itFix all issues by making it a member function.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, achow101 |
| Concept ACK | fanquake |
| Stale ACK | stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
122@@ -123,7 +123,7 @@ class UniValue {
123 const UniValue& get_array() const;
124
125 enum VType type() const { return getType(); }
126- friend const UniValue& find_value( const UniValue& obj, const std::string& name);
127+ const UniValue& find_value(std::string_view key) const;
LIFETIMEBOUND?
Ubuntu 23.04 using GCC 13.0.1
I think it ships with 12.2? https://packages.ubuntu.com/lunar/gcc
ACK fa0d180f48d4e5253f58aced32768369f963d1c7
I can’t see any behaviour change, and as far as I can tell using references in fa0d180f48d4e5253f58aced32768369f963d1c7 doesn’t introduce any (potential) lifetime issues.
Ubuntu 23.04 using GCC 13.0.1
I think it ships with 12.2? packages.ubuntu.com/lunar/gcc
https://packages.ubuntu.com/lunar/g++-13 has been installed as sudo apt install g++-13.
With fa0d180f48d4e5253f58aced32768369f963d1c7 & gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1), fixes all issues from #26926 except for:
0test/interfaces_tests.cpp: In member function ‘void interfaces_tests::findCommonAncestor::test_method()’:
1test/interfaces_tests.cpp:101:19: warning: possibly dangling reference to a temporary [-Wdangling-reference]
2 101 | const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
3 | ^~~~~~
4In file included from ./common/args.h:9,
5 from ./test/util/setup_common.h:9,
6 from test/interfaces_tests.cpp:9:
7./sync.h:302:96: note: the temporary was destroyed at the end of the full expression ‘<lambda closure object>interfaces_tests::findCommonAncestor::test_method()::<lambda()>{((interfaces_tests::findCommonAncestor*)this)}.interfaces_tests::findCommonAncestor::test_method()::<lambda()>()’
8 302 | #define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())
9 | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
10test/interfaces_tests.cpp:101:28: note: in expansion of macro ‘WITH_LOCK’
11 101 | const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
12 | ^~~~~~~~~
13 CXX test/test_bitcoin-netbase_tests.o
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i 's/find_value\(([^ ,]+), /\1.find_value(/g' $(git grep -l find_value)
-END VERIFY SCRIPT-
This can be reverted once gcc excludes lambdas with decltype(auto)
return type from its -Wdangling-reference analysis.
except for …
Might be good to ask the gcc devs if they want to fix this, as it seems a different false positive for decltype(auto) return type lambdas?
Still, added a temporary commit here to hack around it.
97@@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
98 BOOST_AUTO_TEST_CASE(findCommonAncestor)
99 {
100 auto& chain = m_node.chain;
101- const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
102+ const CChain& active{*WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return &Assert(m_node.chainman)->ActiveChain())};
I think it would be best to add the commit message as code documentation to make it easier to track when this can be reverted?
I’m also not sure this commit fits the scope of the PR but I can see how it’s related, and it’s a separate commit anyway so probably fine.
As the test is executed in a single thread, is it a big deal to hold ::cs_main during the entire test case?
Why not to use the pattern as https://github.com/bitcoin/bitcoin/blob/fa266c4bbf564308ddbc12653527226506902084/src/test/interfaces_tests.cpp#L89-L91 or https://github.com/bitcoin/bitcoin/blob/fa266c4bbf564308ddbc12653527226506902084/src/test/interfaces_tests.cpp#L130-L132 ?
Likely due to a LOCKS_EXCLUDED?
Right, in Chainstate::InvalidateBlock.
#define WITH_LOCK?
Maybe notice the GCC-13 bug in the comments for #define WITH_LOCK?
I think #27605 (review) applies and we should disable the warning instead if there are more false positives?
