Merge #20495: sync: Use decltype(auto) return type for WITH_LOCK

3eb94ec81b72b14f72a1f6ce5c9aa24476df755a sync: Use decltype(auto) return type for WITH_LOCK (Carl Dong)

Pull request description:

  > Now that we're using C++17, we can use the decltype(auto) return type
  > for functions and lambda expressions.
  >
  > As demonstrated in this commit, this can simplify cases where previously
  > the compiler failed to deduce the correct return type.
  >
  > Just for reference, for the "assign to ref" cases fixed here, there are
  > 3 possible solutions:
  >
  > - Return a pointer and immediately deref as used before this commit
  > - Make sure the function/lambda returns declspec(auto) as used after
  >   this commit
  > - Class& i = WITH_LOCK(..., return std::ref(...));
  >
  > -----
  >
  > References:
  > 1. https://en.cppreference.com/w/cpp/language/function#Return_type_deduction
  > 2. https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts
  > 3. https://en.cppreference.com/w/cpp/language/auto
  > 4. https://en.cppreference.com/w/cpp/language/decltype
  >
  > Explanations:
  > 1. https://stackoverflow.com/a/21369192
  > 2. https://stackoverflow.com/a/21369170

  Thanks to sipa and ryanofsky for helping me understand this

ACKs for top commit:
  jnewbery:
    utACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a
  hebasto:
    ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings:
  ryanofsky:
    Code review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a

Tree-SHA512: 5f55c7722aeca8ea70e5c1a8db93e93ba0e356e8967e7f607ada38003df4b153d73c29bd2cea8d7ec1344720d37d857ea7dbfd2a88da1d92e0e9cbb9abd287df

3 files changed, 21 insertions, 6 deletions

diff --git a/src/sync.h b/src/sync.h
index 749bf5575c..53213c2089 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -258,7 +258,22 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
 //!
 //!   int val = WITH_LOCK(cs, return shared_val);
 //!
-#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
+//! Note:
+//!
+//! Since the return type deduction follows that of decltype(auto), while the
+//! deduced type of:
+//!
+//!   WITH_LOCK(cs, return {int i = 1; return i;});
+//!
+//! is int, the deduced type of:
+//!
+//!   WITH_LOCK(cs, return {int j = 1; return (j);});
+//!
+//! is &int, a reference to a local variable
+//!
+//! The above is detectable at compile-time with the -Wreturn-local-addr flag in
+//! gcc and the -Wreturn-stack-address flag in clang, both enabled by default.
+#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
 
 class CSemaphore
 {
diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index c8a375275f..92d8cf2e7d 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -35,7 +35,7 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
         return outp;
     };
 
-    CChainState& c1 = *WITH_LOCK(cs_main, return &manager.InitializeChainstate(mempool));
+    CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool));
     c1.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
     WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 75939e0140..3d8570e27c 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 
     // Create a legacy (IBD) chainstate.
     //
-    CChainState& c1 = *WITH_LOCK(::cs_main, return &manager.InitializeChainstate(mempool));
+    CChainState& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(mempool));
     chainstates.push_back(&c1);
     c1.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
@@ -56,7 +56,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 
     // Create a snapshot-based chainstate.
     //
-    CChainState& c2 = *WITH_LOCK(::cs_main, return &manager.InitializeChainstate(mempool, GetRandHash()));
+    CChainState& c2 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(mempool, GetRandHash()));
     chainstates.push_back(&c2);
     c2.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
@@ -116,7 +116,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
 
     // Create a legacy (IBD) chainstate.
     //
-    CChainState& c1 = *WITH_LOCK(cs_main, return &manager.InitializeChainstate(mempool));
+    CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool));
     chainstates.push_back(&c1);
     c1.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
@@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
 
     // Create a snapshot-based chainstate.
     //
-    CChainState& c2 = *WITH_LOCK(cs_main, return &manager.InitializeChainstate(mempool, GetRandHash()));
+    CChainState& c2 = WITH_LOCK(cs_main, return manager.InitializeChainstate(mempool, GetRandHash()));
     chainstates.push_back(&c2);
     c2.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);