lint: Remove string exclusion from locale check #32434

The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

For example, the following is not detected:

diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
 RPCHelpMan keypoolrefill()
 {
     return RPCHelpMan{"keypoolrefill",
-                "\nFills the keypool."+
+                "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"
+                "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
         HELP_REQUIRING_PASSPHRASE,
                 {
                     {"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", DEFAULT_KEYPOOL_SIZE)}, "The new keypool size"},

Fix the script by detecting it.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32434.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

It seems to.

➜  bitcoin git:(keypoolrefill_docs) ✗ ./test/lint/lint-locale-dependence.py
The locale dependent function std::to_string(...) appears to be used:
src/wallet/rpc/addresses.cpp:                "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
src/wallet/rpc/addresses.cpp:                    {"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", /* bla */ std::to_string(DEFAULT_KEYPOOL_SIZE))}, "The new keypool size"},

Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible.

Advice not applicable in this specific case? Add an exception by updating the ignore list in ./test/lint/lint-locale-dependence.py

Maybe can mention the to_string function in the docs as well? It's already a part of the linter.

https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/doc/developer-notes.md?plain=1#L1022-L1042

The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

Yes, I can see it may happen with named args. Fixed.

Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy

Maybe can mention the to_string function in the docs as well? It's already a part of the linter.

I removed it, as per the rationale in the commit message. (There are many more missing than just this one)

Code review ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b.

Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy

Agree. The current script is more of a band-aid, there are likely better ways to check if certain functions aren't used in C++ code.

What came to mind is "just check the linker output, the linker knows". However this won't catch inline / template functions.

ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b

I have not hand-checked the regexs but I did run the linter with the changes in #32429 and the errors were correctly caught.

The earlier documentation made me wonder whether the dev is supposed to run this lint manually but I verified that it is a part of the CI as well, just like the other python linters.

+1 on the detailed reasoning in the commit message regarding removal in the docs.

ACK https://github.com/bitcoin/bitcoin/pull/32434/commits/fa24fdcb7f474e6959ae4d8fe9759d365c6c021b