From 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a Mon Sep 17 00:00:00 2001
From: Carl Dong <contact@carldong.me>
Date: Wed, 25 Nov 2020 11:05:23 -0500
Subject: sync: Use decltype(auto) return type for WITH_LOCK

Now that we're using C++17, we can use the decltype(auto) return type
(available since C++14) 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
3. Item 3 in Effective Modern C++ (Scott Meyers) via jnewbery
---
 src/sync.h                                      | 17 ++++++++++++++++-
 src/test/validation_chainstate_tests.cpp        |  2 +-
 src/test/validation_chainstatemanager_tests.cpp |  8 ++++----
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/sync.h b/src/sync.h
index 0948083c7f..faabec2c2d 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -256,7 +256,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 36badafc4e..a6ec4ce17f 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -29,7 +29,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);
@@ -55,7 +55,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);
@@ -114,7 +114,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);
@@ -131,7 +131,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);
-- 
cgit v1.2.3