sync: Use decltype(auto) return type for WITH

dongcarl commented at 4:33 pm on November 25, 2020: member

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:

https://en.cppreference.com/w/cpp/language/function#Return_type_deduction

https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts

https://en.cppreference.com/w/cpp/language/auto

https://en.cppreference.com/w/cpp/language/decltype

Explanations:

https://stackoverflow.com/a/21369192

https://stackoverflow.com/a/21369170

Thanks to sipa and ryanofsky for helping me understand this

DrahtBot added the label Tests on Nov 25, 2020

DrahtBot commented at 3:23 am on November 26, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19806 (validation: UTXO snapshot activation by jamesob)

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.

jnewbery commented at 11:11 am on November 26, 2020: member

Code review ACK 50dee5a61d02eecaf78e5ac5e0305477478e5ed3

Thanks for the excellent explanation!

It’s not important, but decltype(auto) has been available since c++14. You could mention that in the commit log if you felt like it.

Another really good reference on decltype(auto) is item 3 in Effective Modern C++ (Scott Meyers). That chapter explains one sharp edge that I wanted to test, namely that decltype(auto(i)) and decltype(auto((i))) are not always the same:

 0#include <iostream>
 1#include <mutex>
 2
 3#define LOCK(cs) (void(cs))
 4#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
 5
 6int main()
 7{
 8    std::mutex l;
 9
10    int i = WITH_LOCK(l, {int i = 1; return i;}); // this is fine, auto(decltype(i)) is an int, so this returns by value
11    std::cout << "i = " << i << std::endl;
12
13    int j = WITH_LOCK(l, {int j = 1; return (j);}); // this isn't, auto(decltype((i)) is an &int, so this returns by reference to a local variable
14    std::cout << "j = " << j << std::endl; // segfault!! j is a dangling reference
15
16    return 0;
17}

This kind of bug is non-obvious from looking at the code, but it’s detected by -Wreturn-local-addr, which I believe we have enabled by default:

0g++ decltype_auto.cpp 
1decltype_auto.cpp: In lambda function:
2decltype_auto.cpp:14:46: warning: reference to local variable ‘j’ returned [-Wreturn-local-addr]
3   14 |     int j = WITH_LOCK(l, {int j = 1; return (j);}); // this isn't, auto(decltype((i)) is an &int, so this returns by reference to a local variable
4      |                                              ^

ajtowns commented at 11:38 pm on December 1, 2020: member

Is this actually clearer or less fragile (or better in some other way?) than having the explicit & and *?

MarcoFalke commented at 8:08 am on December 2, 2020: member

I think the only benefit is when new code is written, it will less likely result in a compile error. So devs need to do the add-*-or&-at-random-places-until-it-compiles workaround less often.

dongcarl force-pushed on Dec 3, 2020

dongcarl commented at 9:33 pm on December 3, 2020: member

Is this actually clearer or less fragile (or better in some other way?) than having the explicit & and *?

My intuition could be wrong, but I think that most people, when calling WITH_LOCK, don’t expect their deducted return type to be “coerced” into an object type. In WITH_LOCK’s particular case, this seem somewhat rigid and affects the ergonomics of a major use of WITH_LOCK: making use of its return value.

dongcarl commented at 9:34 pm on December 3, 2020: member

Pushed 50dee5a61d02eecaf78e5ac5e0305477478e5ed3 to 7a8f4b9ea82a44c8512e6c6723ffb666cb585115: put some more context into the repo. Thanks jnewbery!

in src/sync.h:270 in 7a8f4b9ea8 outdated

264+//!
265+//! is int, the deducted type of:
266+//!
267+//!   WITH_LOCK(cs, return {int j = 1; return (j);});
268+//!
269+//! is &int, a reference to a local variable

promag commented at 9:50 pm on December 3, 2020:

TIL 😱

ryanofsky commented at 10:22 pm on December 3, 2020:

TIL

Wow, I didn’t know this either. It’s pretty funny. Seems to be C++’s way of providing two functions for the price of one keyword. decltype(variable) gives you declared type of variable while decltype(expression) gives you type of the expression with an extra & or && added to indicate lvalue or rvalueness (https://stackoverflow.com/questions/27557369/why-does-decltypeauto-return-a-reference-here/27557450#27557450)

promag commented at 9:51 pm on December 3, 2020: member

Code review ACK 7a8f4b9ea82a44c8512e6c6723ffb666cb585115, could squash.

promag commented at 10:27 pm on December 3, 2020: member

@jnewbery I get a different warning:

0$ clang++ -std=c++17 decltype_auto.cpp
1decltype_auto.cpp:14:46: warning: reference to stack memory associated with local variable 'j' returned [-Wreturn-stack-address]
2    int j = WITH_LOCK(l, {int j = 1; return (j);}); // this isn't, auto(decltype((i)) is an &int, so this returns by reference to a local variable
3                                             ^
4decltype_auto.cpp:5:65: note: expanded from macro 'WITH_LOCK'
5#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
6                                                                ^~~~
71 warning generated.

And doesn’t segfault.

in src/sync.h:260 in 7a8f4b9ea8 outdated

253@@ -254,7 +254,22 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
254 //!
255 //!   int val = WITH_LOCK(cs, return shared_val);
256 //!
257-#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
258+//! Note:
259+//!
260+//! Since the return type deduction follows that of decltype(auto), while the
261+//! deducted type of:

jnewbery commented at 11:26 am on December 4, 2020:

s/deducted/deduced/

in src/sync.h:264 in 7a8f4b9ea8 outdated

260+//! Since the return type deduction follows that of decltype(auto), while the
261+//! deducted type of:
262+//!
263+//!   WITH_LOCK(cs, return {int i = 1; return i;});
264+//!
265+//! is int, the deducted type of:

jnewbery commented at 11:26 am on December 4, 2020:

as above, deducted->deduced

jnewbery commented at 11:28 am on December 4, 2020: member

utACK 7a8f4b9ea8.

I’m +/-0 on adding the comment about decltype, since I don’t think we need to document every quirk of c++ in our codebase. I don’t think it does any harm though.

dongcarl force-pushed on Dec 4, 2020

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

3eb94ec81b

dongcarl force-pushed on Dec 4, 2020

hebasto approved

hebasto commented at 12:53 pm on December 5, 2020: member

ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings:

-Wreturn-stack-address in clang
-Wreturn-local-addr in gcc

I think this change makes reading code which uses the WITH_LOCK macro and reasoning about it easier.

jnewbery commented at 10:22 am on December 7, 2020: member

utACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a

ryanofsky approved

ryanofsky commented at 11:41 pm on December 7, 2020: member

Code review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a

fanquake merged this on Jan 12, 2021

fanquake closed this on Jan 12, 2021

sidhujag referenced this in commit 9eb98d7718 on Jan 12, 2021

MarcoFalke commented at 8:41 pm on June 18, 2021: member

review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a

DrahtBot locked this on Aug 18, 2022

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

Conflicts