Add dev guideline limiting auto usage.

TheBlueMatt commented at 7:33 pm on January 8, 2018: member

I’m tired of having an extra level of indirection to review stupid shit like https://github.com/bitcoin/bitcoin/pull/11403/commits/5ae4080cc2c0c50e1093b52005c47cd74bd0261e#diff-ad6efdc354b57bd1fa29fc3abb6e2872R283, it just makes review, scripted-diff and other stuff harder, without materially making the code more flexible (unless we want to do a massive sed to make everything auto, which would be insane).

TheBlueMatt force-pushed on Jan 8, 2018

fanquake added the label Docs on Jan 8, 2018

jonasschnelli approved

jonasschnelli commented at 6:38 am on January 9, 2018: contributor

ACK 9398b3d2dbaa35f1f029d4072dad9282920430c8

in doc/developer-notes.md:42 in 9398b3d2db outdated

38@@ -39,6 +39,7 @@ code.
39   - `++i` is preferred over `i++`.
40   - `nullptr` is preferred over `NULL` or `(void*)0`.
41   - `static_assert` is preferred over `assert` where possible. Generally; compile-time checking is preferred over run-time checking.
42+  - Do not use `auto` unless it saves significant typing/reading or in method signatures where it makes the method more flexible.

promag commented at 10:36 am on January 9, 2018:

µnit Only use `auto` if it ...

promag commented at 10:37 am on January 9, 2018: member

ACK.

sipa commented at 10:45 am on January 9, 2018: member

“auto” in method signatures in C++11 is only allowed when still specifying the return type explicitly (auto fn(args…) -> returntype notation). Is that what you’re referring to?

Add dev guideline limiting auto usage. 6e1fa4e50d

TheBlueMatt force-pushed on Jan 9, 2018

TheBlueMatt commented at 9:55 pm on January 9, 2018: member

Ok, amended to say “or if it is critical to the functionality of the code”, nice big loophole but if it really matters somewhere of course people can use it.

MarcoFalke commented at 10:16 am on January 10, 2018: member

ACK 6e1fa4e50d99803b7150891cc53761191303bacf

ajtowns commented at 2:15 am on January 11, 2018: member

This seems like it would be better if it was a bit more explicit/objective about what’s acceptable:

Use of auto outside of unit tests should be limited to the following circumstances in order to avoid hiding type information: * range-based for loops, eg for (const auto& i : vFoo) * when dealing with iterators in general, eg auto it = vFoo.rbegin() * when creating a closure/lambda * in templates, where necessary * when using a variable as read-only alias of a long expression, eg const auto x = long(expression())

Presumably overusing auto in unit tests isn’t a big deal and the first four bullet points are uncontroversial?

Presumably the last one is what irritates @TheBlueMatt, so shouldn’t be there, and places in the code that currently use it should get converted to specifying an actual type.

FWIW, in current master, usage of auto seems to pretty much match up with the list above so not having loopholes seems practical:

A bunch of uses in C++11 for loops (133):
- for (const auto& foo : bar) – 7 in *.h, 94 in *.cpp
- for (auto&& foo : bar) and for (auto& foo:bar)– 4 in net.h, 1 in undo.h, 16 in *.cpp
- for (auto foo : bar) – 1 in reverse_iterator.h, 9 in *.cpp
- for (const auto foo : bar) – 1 in rpc/blockchain.cpp
A bunch of uses with iterators (72):
- auto it = foo.find(bar) – 47 in *.cpp
- auto it = foo.begin() (or rbegin) – 11 in *.cpp
- for (auto x = foo.begin(); ..) (or rbegin) – 6 in *.cpp
- another 8 that are probably iterator related:

0./support/lockedpool.cpp:    auto it = std::find_if(chunks_free.begin(), chunks_free.end(),
1./support/lockedpool.cpp:    auto next = chunks_free.upper_bound(freed.first);
2./support/lockedpool.cpp:    auto alloced = chunks_used.emplace(it->first + it->second - size, size).first;
3./txmempool.cpp:        auto iter = mapNextTx.lower_bound(COutPoint(hash, 0));
4./txmempool.cpp:        auto iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0));
5./txmempool.cpp:    auto iters = GetSortedDepthAndScore();
6./txmempool.cpp:    auto iters = GetSortedDepthAndScore();
7./wallet/wallet.cpp:    auto iter = mapTxSpends.lower_bound(COutPoint(txid, 0));

Shortcuts in bench/ and test/ (48)
- for (auto x = 0; …) – 12 of them.. weird
- 36 others
Aliasing (22)
- const auto foo = bar; – 22 in *.cpp
Usage with closures/lambdas (4):
- auto foo = […](…) { … } – 4 in *.cpp
Usage with templates (3):
- template return values – 3 in reverse_iterator.h
And 14 others in non-bench non-test *.cpp:

 0./base58.cpp:    auto bech = bech32::Decode(str);     // pair<string,vector<uint8>>
 1./httpserver.cpp:    auto req_copy = req;    // struct evhttp_request *
 2./net_processing.cpp:    auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME});  // pair<iterator,bool>
 3./net_processing.cpp:                auto txinfo = mempool.info(inv.hash);  // TxMempoolInfo
 4./net_processing.cpp:                auto vtxinfo = mempool.infoAll();  // vector<TxMempoolInfo>
 5./net_processing.cpp:                    auto txinfo = mempool.info(hash);  // TxMempoolInfo
 6./net_processing.cpp:                        auto ret = mapRelay.insert(std::make_pair(hash, std::move(txinfo.tx))); // pair<iterator,bool>
 7./qt/optionsmodel.cpp:            auto ip_port = GetProxySetting(settings, "addrProxy"); // ProxySetting
 8./qt/optionsmodel.cpp:            auto ip_port = GetProxySetting(settings, "addrProxy");
 9./qt/optionsmodel.cpp:            auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
10./qt/optionsmodel.cpp:            auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
11./qt/paymentserver.cpp:                auto tempChainParams = CreateChainParams(CBaseChainParams::MAIN); // unique_ptr<CChainParams>
12./qt/walletmodel.cpp:        auto& resultGroup = mapCoins[QString::fromStdString(EncodeDestination(group.first))]; // COutput
13./support/lockedpool.cpp:    auto freed = *i; // pair<char* const, size_t>

TheBlueMatt commented at 4:19 pm on January 11, 2018: member

Honestly I’m really not a big fan of use in for loops either.
I don’t think “iterators in general” is a reasonable metric, the metric is still readability-based - if the iterator is short (which they rarely are) it could still be more readable to type it out, if the iterator is a mile long (which they usually are) its obviously less readable.
Same for a lambda.
I’d strongly prefer if nothing used auto in return types, not just in templates. It makes review much, much harder.
Again, its all about readability. Just because an expression is long does not mean it should use auto, the example I cited was to use auto for this precise reason but I found it quite annoying.

As for unit tests, I dont see why unit tests should be an exception, though we often simply dont apply style guidelines on unit tests anyway just because they have a lower bar.

ajtowns commented at 0:11 am on January 12, 2018: member

The main difference I see between “auto x = FunctionCall(foo);” and “auto x = foo.begin()” or “for (auto x : foo)” is you can get a good expectation of the inferred type locally – you just have to look back to the recent declaration of foo to know what convention implies the type of x will be; while with the function call you have to search header files for FunctionCall’s declaration.

Lambdas/closures are similar in that their definition is right there, with the added problem that they have anonymous types in the first place, and apparently converting them to something you can write out has a performance hit.

I don’t see what’s ambiguous or confusing about “auto begin() const -> decltype(this->m_x.rbegin())”, or how it’s hard to review, or how you’d rewrite it to be better? C++14 type inference from the returned value would be worse though.

I do agree with your rationale here (more so after reviewing how auto’s actually used), but “readability” is very subjective – if I weren’t following this discussion, I’d probably think “const auto& x = Blah();” “saves significant reading” compared to “const std::map<std::string,std::vectorstd::string>& x = Blah();” and end up violating the style guidelines while trying to comply with them. Getting called out for it during review would then be kind of irritating.

For the record, I had a go at a patch to switch unnecessary uses of auto to explicit types, at https://github.com/TheBlueMatt/bitcoin/pull/9

theuni commented at 10:06 pm on January 12, 2018: member

I generally disagree with discouraging ‘auto’ unless it completely obscures away the type.

See #12169 for a perfect example of the types of mistakes it can obsolete.

laanwj commented at 8:17 am on January 15, 2018: member

I generally disagree with discouraging ‘auto’ unless it completely obscures away the type.

I agree here. auto was a great addition, it both saves typing and verbosity while reading code. Yes, sometimes auto makes reviewing harder, in those specific cases do make a review comment, but right now this is a blanket discouragement that doesn’t give any specific advice or rationale. So tend to NACK.

If you’d describe in the document specific uses of auto that you disagree with, and why, I think this could be a more useful discussion.

TheBlueMatt commented at 6:45 pm on January 15, 2018: member

it both saves typing and verbosity while reading code. this is a blanket discouragement that doesn’t give any specific advice or rationale

Huh? This isn’t a “blanket discouragement”, it specifically calls out places where readability is better as places where auto is fine. I’m happy to reword it if you think it reads as a blanket discouragement, but that isn’t the point.

On the other hand, I find the “readability” argument suspect except for cases like iterators and anonymous functions, though the “no implicit conversion” feature is quite nice, I’m less-than-convinced its worth trading being explicit to get it.

laanwj commented at 1:16 pm on January 29, 2018: member

This isn’t a “blanket discouragement”

To me it reads like a blanket discouragement, because you are just adding a line and not quoting any specific cases. It amounts to “don’t use language feature X unnecessarily” which should be unnecessary to state.

MarcoFalke commented at 7:11 pm on February 10, 2018: member

Closing for now due to inactivity

MarcoFalke closed this on Feb 10, 2018

DrahtBot locked this on Sep 8, 2021

Add dev guideline limiting auto usage. #12120