refactor: Use std::string for thread and index names

stickies-v commented at 11:26 am on September 1, 2022: contributor

As a follow-up to #25967 (review), this PR changes the return type of BaseIndex::GetName() to const std::string& instead of const char*. The first commit is not essential for this change, but since the code is touched and index names are commonly used to specify thread names, I’ve made the same update there.

No behaviour change, just refactoring to further phase out C-style strings.

Note: util::ThreadRename() used to take an rvalue ref, but since it then passes this to SetInternalName() by value, I don’t think there’s any benefit to having both an rvalue and lvalue ref function so I just changed it into lvalue ref. Not 100% sure I’m missing something?

fanquake added the label Refactoring on Sep 1, 2022

in src/index/base.h:8 in 22eaf5b5bb outdated

4@@ -5,6 +5,8 @@
5 #ifndef BITCOIN_INDEX_BASE_H
6 #define BITCOIN_INDEX_BASE_H
7 
8+#include <string>

jonatack commented at 11:37 am on September 1, 2022:

We place stdlib include headers after the ones to our codebase.

stickies-v commented at 11:49 am on September 1, 2022:

Are there any written guidelines on this? I couldn’t find anything in the developer notes and noticed both conventions being used, e.g. blockfilter.h for the opposite. Browsing through some more header files, it seems most indeed include system headers last, so I’ll update it.

willcl-ark commented at 12:00 pm on September 1, 2022:

Could be a good addition to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization if it’s generally-agreed practice

stickies-v commented at 12:05 pm on September 1, 2022:

Agreed, would be a good addition. Updated all 4 index header files to import system headers last.

in src/index/coinstatsindex.h:58 in 22eaf5b5bb outdated

50@@ -49,7 +51,11 @@ class CoinStatsIndex final : public BaseIndex
51 
52     BaseIndex::DB& GetDB() const override { return *m_db; }
53 
54-    const char* GetName() const override { return "coinstatsindex"; }
55+    const std::string& GetName() const override
56+    {
57+        static std::string name{"coinstatsindex"};
58+        return name;
59+    }

jonatack commented at 11:38 am on September 1, 2022:

Can the temporary here (and in txindex.h) be omitted? (also, why static)

stickies-v commented at 11:54 am on September 1, 2022:

We need to return a const std::string& reference, so the underlying object can’t be destroyed after the function returns, which is achieved with the static. I don’t know of a better way to implement this but I’m open to suggestions?

jonatack commented at 12:41 pm on September 1, 2022:

Ah, right.

jonatack commented at 1:15 pm on September 1, 2022:

 0diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
 1index 91a30fb9d9..609bbcdab6 100644
 2--- a/src/index/coinstatsindex.h
 3+++ b/src/index/coinstatsindex.h
 4@@ -19,6 +19,7 @@
 5 class CoinStatsIndex final : public BaseIndex
 6 {
 7 private:
 8+    const std::string index_name{"coinstatsindex"};
 9     std::string m_name;
10     std::unique_ptr<BaseIndex::DB> m_db;
11 
12@@ -51,11 +52,7 @@ protected:
13 
14     BaseIndex::DB& GetDB() const override { return *m_db; }
15 
16-    const std::string& GetName() const override
17-    {
18-        static std::string name{"coinstatsindex"};
19-        return name;
20-    }
21+    const std::string& GetName() const override { return index_name; }
22 
23 public:
24     // Constructs the index, which becomes available to be queried.
25diff --git a/src/index/txindex.h b/src/index/txindex.h
26index cf0ecf498f..ec6ea1d55b 100644
27--- a/src/index/txindex.h
28+++ b/src/index/txindex.h
29@@ -20,6 +20,7 @@ protected:
30     class DB;
31 
32 private:
33+    const std::string index_name{"txindex"};
34     const std::unique_ptr<DB> m_db;
35 
36     bool AllowPrune() const override { return false; }
37@@ -29,11 +30,7 @@ protected:
38 
39     BaseIndex::DB& GetDB() const override;
40 
41-    const std::string& GetName() const override
42-    {
43-        static std::string name{"txindex"};
44-        return name;
45-    }
46+    const std::string& GetName() const override { return index_name; }
47 
48 public:

(Edit: making the index_name definitions const is optional)

stickies-v commented at 1:27 pm on September 1, 2022:

No strong feelings, but I’m not sure it’s an improvement:

doesn’t reduce the amount of code (disregarding newlines)
moves the name further away from the only place where it’s used, so imo slightly harder to read
requires index_name to be initialized for every instance of the class instead of just once

So with that in mind, I think I’d rather keep it as is?

MarcoFalke commented at 1:48 pm on September 1, 2022:

It may also be possible to return a string_view (from a lifetime perspective this is no different than a reference), which works for strings and char-string literals.

string_view should work with tinyformat as well as std::string::operator+ (edit: not operator+, but append)

stickies-v commented at 1:54 pm on September 1, 2022:

I considered that, but are you sure about string_view having implemented operator+? It’s not in the reference. Mostly because of the lack of concatenation, I thought it’d be best for these getters to use a const string& to not make too many assumptions around how the return values will be used.

Edit: to clarify, for example in util::ThreadRename, when passed a string_view you wouldn’t be able to concatenate "b-" + name, would have to be "b-" + name.data() https://github.com/bitcoin/bitcoin/blob/fa5c224d444802dabec5841009e029b9754c92f1/src/util/threadnames.cpp#L59-L63

That’s not a huge issue on its own, but I felt the const string& approach was a bit cleaner partly for that reason.

sipa commented at 1:58 pm on September 1, 2022:

You can’t concatenate string_views, as string_views are pointer/length references to continuous characters in memory. The concatenation of two is not continuous anymore.

~~That doesn’t matter though; a string_view can be implicitly converted to a string, so concatenating a string to a string_view will yield a string.~~ EDIT: I’m wrong.

That said, I don’t think string_view is much of a benefit here; it’s not like these names get sliced. See my suggestion below.

jonatack commented at 2:14 pm on September 1, 2022:

 0diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
 1index b9029e946a..a554cd4e3b 100644
 2--- a/src/index/coinstatsindex.cpp
 3+++ b/src/index/coinstatsindex.cpp
 4@@ -23,6 +23,7 @@ using node::UndoReadFromDisk;
 5 static constexpr uint8_t DB_MUHASH{'M'};
 6+static const std::string index_name{"coinstatsindex"};
 7 
 8@@ -392,6 +393,8 @@ bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockKey>& block
 9 
10+const std::string& CoinStatsIndex::GetName() const { return index_name; }
11+
12 bool CoinStatsIndex::CustomCommit(CDBBatch& batch)
13diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
14index 91a30fb9d9..54fff61e53 100644
15--- a/src/index/coinstatsindex.h
16+++ b/src/index/coinstatsindex.h
17@@ -51,11 +51,7 @@ protected:
18 
19-    const std::string& GetName() const override
20-    {
21-        static std::string name{"coinstatsindex"};
22-        return name;
23-    }
24+    const std::string& GetName() const override;
25 
26 public:
27     // Constructs the index, which becomes available to be queried.
28diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
29index b719aface8..489bcd53c0 100644
30--- a/src/index/txindex.cpp
31+++ b/src/index/txindex.cpp
32@@ -12,6 +12,7 @@
33 constexpr uint8_t DB_TXINDEX{'t'};
34+static const std::string index_name{"txindex"};
35  
36@@ -54,6 +55,8 @@ TxIndex::TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size,
37 
38+const std::string& TxIndex::GetName() const { return index_name; }
39+
40 bool TxIndex::CustomAppend(const interfaces::BlockInfo& block)
41 {
42     // Exclude genesis block transaction because outputs are not spendable.
43diff --git a/src/index/txindex.h b/src/index/txindex.h
44index cf0ecf498f..c54ad562d8 100644
45--- a/src/index/txindex.h
46+++ b/src/index/txindex.h
47@@ -29,11 +29,7 @@ protected:
48 
49-    const std::string& GetName() const override
50-    {
51-        static std::string name{"txindex"};
52-        return name;
53-    }
54+    const std::string& GetName() const override;
55 
56 public:
57 ``
58</p></details>

MarcoFalke commented at 2:18 pm on September 1, 2022:

Edit: to clarify, for example in util::ThreadRename, when passed a string_view you wouldn’t be able to concatenate “b-” + name, would have to be “b-” + name.data()

"b-" + name.data() is undefined behaviour if it compiles.

I was thinking that you’d be able to write "b-"s + name, but I was wrong.

Edit: To clarify https://en.cppreference.com/w/cpp/string/basic_string/append does accept it, so you could write

std::string{"b-"}.append(name)

stickies-v commented at 4:58 pm on September 1, 2022:

Thank you for the input everyone, much appreciated!

I’ve incorporated sipa’s suggestion to move the name logic to BaseIndex, which I think is an elegant solution and should hopefully also satisfy jonatack’s concerns? The only downside to this approach I see is that there is some code duplication in the BlockFilterIndex constructor as we need to derive the filter name from the filter type, but it’s minimal enough imo.

I’ve also updated the thread name parameters to string_view wherever it made sense.

so you could write…

Thanks for the clarification! The call to SetInternalName() on the next line also requires a string, so I think it’s best to just expect a string type as parameter (reverting it to what it was before).

I think this addresses everyone’s concerns voiced here, but I’ll leave the comment open for a bit longer so people can react if I misunderstood anything.

MarcoFalke commented at 4:25 pm on September 2, 2022:

Closing this thread as resolved

stickies-v force-pushed on Sep 1, 2022

theStack commented at 12:11 pm on September 1, 2022: contributor

Concept ACK

brunoerg commented at 1:00 pm on September 1, 2022: contributor

Concept ACK

sipa commented at 1:43 pm on September 1, 2022: member

What about:

Adding an const std::string m_name; field to the base class.
Require a name to be passed in to the base class constructor (invoked from the child class constructors).
Adding a non-virtual accessor const std::string& GetName() const { return m_name; } to return that name.
Remove the corresponding m_name / GetName from the BlockFilterIndex class.
Make all child classes pass a name to the parent class constructor.

stickies-v force-pushed on Sep 1, 2022

stickies-v commented at 5:00 pm on September 1, 2022: contributor

Force pushed to address review comments about simplifying the GetName() logic and using string_view instead of const std::string&

in src/util/thread.cpp:16 in a6927e8993 outdated

 9@@ -10,10 +10,11 @@
10 
11 #include <exception>
12 #include <functional>
13+#include <string>
14 
15-void util::TraceThread(const char* thread_name, std::function<void()> thread_func)
16+void util::TraceThread(std::string_view thread_name, std::function<void()> thread_func)

MarcoFalke commented at 5:20 pm on September 1, 2022:

0void util::TraceThread(std::string thread_name, std::function<void()> thread_func)

I think the overhead of creating a thread is much higher than creating a copy of a few bytes. And having not to think about lifetime (or run into lifetime issues in never expected corner cases) just isn’t worth the questionable benefits here that are given by a char* or string_view.

stickies-v commented at 1:25 pm on September 2, 2022:

I agree, thanks, good catch. Made it a const std::string instead, and since we’re already copying here I re-reverted the logic for ThreadRename() to use an lvalue ref again.

sipa commented at 1:35 pm on September 2, 2022:

You probably want a const std::string& to avoid a copy.

stickies-v commented at 1:40 pm on September 2, 2022:

Do you mean for util::TraceThread()? I think we explicitly want a copy here to not have to worry about lifetime as per MarcoFalke’s comment? The copying I mentioned was further downstream for when we pass thread_name to util::ThreadRename(), where indeed a const std::string& is used to avoid copying again.

sipa commented at 1:43 pm on September 2, 2022:

Is there a reference stored to this thread_name variable somewhere? If not, there cannot be any lifetime issues.

MarcoFalke commented at 1:45 pm on September 2, 2022:

TraceThread does not return before the end of the thread, so the reference is implicitly stored as a bound argument for the whole duration of the thread, no?

sipa commented at 1:48 pm on September 2, 2022:

If TraceThread doesn’t return before the thread is done executing, then the object the caller references can’t disappear before the thread exits (because the caller doesn’t even get to run) and there cannot be any lifetime issues.

stickies-v commented at 3:26 pm on September 2, 2022:

@MarcoFalke if you agree I’ll revert to string_view again, I’m not feeling very confident here but what sipa says makes sense to me. At the very least, it shouldn’t introduce any new lifetime issues compared to what we had before with const char*?

MarcoFalke commented at 3:50 pm on September 2, 2022:

Oh, I see. The arguments to std::thread are copied, unless they are passed as std::ref ( https://en.cppreference.com/w/cpp/thread/thread/thread )

So const std::string& is indeed fine here, as it is a reference to the copy that was created by the std::thread constructor. (However, std::string should be fine as well and come with only the overhead equivalent to constructing an empty std::string)

MarcoFalke commented at 3:54 pm on September 2, 2022:

For reference, the following gives an AddressSanitizer: stack-use-after-scope, because the argument was passed with std::ref, but does not if the argument is not passed with std::ref.

 0#include <chrono>
 1#include <iostream>
 2#include <thread>
 3
 4void fun(const std::string &a) {
 5  std::this_thread::sleep_for(std::chrono::milliseconds{10});
 6  std::cout << a << std::endl;
 7}
 8
 9int main() {
10  std::thread a;
11  {
12    std::string s{"s"};
13    a = std::thread{fun, std::ref(s)};
14  }
15  a.join();
16}

MarcoFalke commented at 4:21 pm on September 2, 2022:

To clarify, anything is fine here, but string_view can lead to lifetime issues if there is a leftover caller that uses c_str on a temporary string. For example, this is UB:

 0#include <chrono>
 1#include <iostream>
 2#include <thread>
 3
 4void fun(std::string_view a) {
 5  std::this_thread::sleep_for(std::chrono::milliseconds{10});
 6  std::cout << a << std::endl;
 7}
 8
 9int main() {
10  std::thread a;
11  {
12    std::string s{"s"};
13    a = std::thread{fun, s.c_str()};
14  }
15  a.join();
16}

stickies-v commented at 4:31 pm on September 2, 2022:

Thanks for the background info MarcoFalke, that’s really helpful.

can lead to lifetime issues if there is a leftover caller that uses c_str on a temporary string

Would you agree that that would just be bad implementation, and not something we should expect downstream functions to try and fix?

MarcoFalke commented at 5:08 pm on September 2, 2022:

Previously passing a std::string was only possible by calling c_str() on it, so this UB may persist in a very unlikely and unfortunate silent merge conflict. (Changing const char* to std::string_view shouldn’t introduce any new UB, but it wouldn’t fix it either, if it existed previously). Though, again that seems unlikely so I think anything is fine here and this is almost a style question.

hebasto commented at 7:46 pm on September 15, 2022:

FWIW, in our code base, all util::TraceThread callers pass either a string literal (in most cases) or a BaseIndex::GetName() return value of const std::string& type (the only case).

DrahtBot commented at 9:35 pm on September 1, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)

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.

in src/index/blockfilterindex.h:15 in a6927e8993 outdated

11@@ -12,6 +12,8 @@
12 #include <index/base.h>
13 #include <util/hasher.h>
14 
15+#include <string>

davidgumberg commented at 3:36 am on September 2, 2022:

Isn’t it the case that we don’t need the #include <string> include here or in txindex.h or coinstatsindex.h?

stickies-v commented at 11:26 am on September 2, 2022:

Yeah thanks, good catch. Removed all 3 includes, leftover from previous versions of the PR.

hebasto commented at 9:13 am on September 2, 2022: member

Concept ACK.

stickies-v force-pushed on Sep 2, 2022

stickies-v commented at 1:30 pm on September 2, 2022: contributor

Force pushed to address review feedback regarding:

util::TraceThread() to take a std::string copy and avoid potential issues with lifetime introduced by the threading, also updated util::ThreadRename() to take a const lvalue ref to avoid copying twice
remove unused includes leftover from earlier versions

bitcoin deleted a comment on Sep 2, 2022

stickies-v force-pushed on Sep 2, 2022

stickies-v commented at 4:22 pm on September 2, 2022: contributor

Force pushed to partly reverse the previous force push based on sipa’s suggestion, again using a string_view for util::TraceThread() and undoing the changes to util::ThreadRename() (now no longer in this PR’s diff).

My apologies to reviewers for the back and forth.

stickies-v force-pushed on Sep 12, 2022

stickies-v commented at 10:19 pm on September 12, 2022: contributor

Force pushed to rebase on latest master to fix failing CI (wallet_groups.py) - no other changes.

MarcoFalke commented at 8:12 am on September 13, 2022: member

My preference would still be to use std::string in TraceThread (https://github.com/bitcoin/bitcoin/pull/25971#discussion_r960917554) as a belt-and-suspenders, but the current code is not worse than it was previously.

review ACK e5d9c6f0571ee182f34e0d82c20e5ca2da30ad85 🎯

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK e5d9c6f0571ee182f34e0d82c20e5ca2da30ad85 🎯
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgoCQv+P5w/6IGCCYaSa+M7unM9uCN6fxnRgqaJlASlzu/rGzN0VJcdS404NTdL
 8FpIpXE2j+kwhgAc6eM+87byb2lR7sNgL/CAC7v1M8tXNMpTMJwlwgLsCykFY7iE9
 9pK6u9BocyloYAcDNw4u7v2Mph7KTxUIXxqZhz9fFR2dPJrxc6eTmP8OXaRiYlbAo
109BxAw79cQY3+2F3V0THByfcgQdyZO7BF2gpZCNgrY84bwIbotsTgtGU9qLpv46V9
112GBgy7v5jUqsFYPVtnM/cs1NVou779N/rCDH2+4uDBiXyYf6VM8d9ZEYIkzayPmC
12hclDiXMwfpZ0hVnbdBhpmYGp9Vb4UoMkZiLleJdlJQsp3R1e3RBCDGPDsy1iV8Aw
13mQMfOkiDQxC91eyizLv3eJ8RnUc7vwE9W+MW6FKQzw1O8vaFKfGJEoIK/nb2aMXr
14Q92DNYhFxzZpQHG5MfoJj20ySJP/woq7xG/7xpH/zW4QuSaxW6OvqZ3GZFUNROGc
15zT0TuUiQ
16=b37U
17-----END PGP SIGNATURE-----

MarcoFalke approved

DrahtBot added the label Needs rebase on Sep 13, 2022

refactor: use std::string for thread names 97f5b20c12

refactor: use std::string for index names 200d84d568

stickies-v force-pushed on Sep 13, 2022

stickies-v commented at 6:14 pm on September 13, 2022: contributor

Force pushed to fix merge conflict from #24513 that renamed CChainstate -> Chainstate - no other changes.

My preference would still be to use std::string in TraceThread (https://github.com/bitcoin/bitcoin/pull/25971#discussion_r960917554) as a belt-and-suspenders, but the current code is not worse than it was previously.

I’m not opposed to it, but as I don’t have a strong view on it, we’ve already had some back-and-forth and the current implementation doesn’t introduce new assumptions there I’d prefer to keep that for a follow-up (which I won’t do).

MarcoFalke commented at 6:23 pm on September 13, 2022: member

re-ACK 200d84d5681918523d982b9ddf60d1127edcb448 only change is rebase 🚼

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 200d84d5681918523d982b9ddf60d1127edcb448 only change is rebase 🚼
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgPNQwAw5LrCWUbRWxPIUc1OLrGT75Vv9alSNady748D38GlxcFc3CBahCxmU4Q
 8sf3vpVbHN0Mo8Y06vNB0mBZ1r4lcL5U73rGzDELCNFEixh08PTELLrcdWCFLk/Dp
 9ScpDdGRyMx4fRkjwaYFbUZsq4qUpfPjYTQBsHKyOn/J9RR/yzbMZOP0IGtonG10t
10jmO52IAPgcD2duZRCY7HZSsJtAmnaFmV0geDH1LdL0j5G2y76Sqxzid/0H6KeoaK
11Tp7lPcf77T/o8lNyYllYAYtEIQp7Gg9yvQOYvmkuAKxyx9f5JOEQtw936zCt5BXt
12DgHD5uve0WnR0vp1v1sowAPci/l8D9fVh9iiWQG8oqHJ8Iha4YLaXicsBrJsEbav
139BR+bIdvUw99Bg8UK1XQdEwczVFYrExhzbP50W04DM3HpD3kjlwdZhvwON6P5XKt
142O8A7wFT+KO4qjno+8DItg1FKeVh9kRuMZ7ptXq4RJVyob3L/jT8gTvKgVkcxj5L
15N0aIxOMu
16=fh6y
17-----END PGP SIGNATURE-----

DrahtBot removed the label Needs rebase on Sep 13, 2022

w0xlt approved

w0xlt commented at 11:29 pm on September 13, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25971/commits/200d84d5681918523d982b9ddf60d1127edcb448

scripted-diff: rename pszThread to thread_name

Since it is now a string_view instead of a const char*, update the
name to reflect that the variable is no longer a "Pointer to
String, Zero-terminated" (psz).

-BEGIN VERIFY SCRIPT-
sed -i s/pszThread/thread_name/ $(git grep -l pszThread src)
-END VERIFY SCRIPT-

26cf9ea8e4

in src/util/system.cpp:834 in 200d84d568 outdated

830@@ -831,7 +831,7 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
831            std::string("\n\n");
832 }
833 
834-static std::string FormatException(const std::exception* pex, const char* pszThread)
835+static std::string FormatException(const std::exception* pex, std::string_view pszThread)

sipa commented at 1:28 am on September 14, 2022:

Nit: the prefix in “pszThread” means “Pointer to String, Zero-terminated”. We don’t use that naming style anymore in new code, and while it’s ok to leave it as is to avoid needing to touch code all over the place, please don’t leave the name when its actual meaning actually mismatches the code.

stickies-v commented at 10:11 am on September 14, 2022:

Oh that’s good to know, thanks. I’ve added 26cf9ea to fix that with a scripted-diff.

MarcoFalke commented at 10:20 am on September 14, 2022: member

review ACK 26cf9ea8e44d7fd6450336f567afaedd1275baf7 only change is new scripted-diff 😀

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 26cf9ea8e44d7fd6450336f567afaedd1275baf7 only change is new scripted-diff 😀
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiQUAwAuj4HULyQMBta0XNIDl0zy4CCXiT1ZBVXSQoMv0mBjQPP7l5m45FhtTQe
 8ThS7P6W0rwQq2jGwmPjucd29gS5LuTuf5tBIoExY17u/TDgKa53wz37ggP5d574V
 9PypsOaugnlpipMrlNBiOguxNS9KjE9n4MrXVF67Pnm+SbcqIW9aOlpLODNYO7MHd
10/T+0wy03Vtmknsze7e4v9xp8HxQ2kZab11auPKSMVfuACDhJEBul9a/xrlDILjTG
11QvfCG2BRdIaUzMLgtLWGTOD8Ilq7h3ObKjnKW4aH42QIRZi/Wfp3tQhvKkOZaW7N
12zAISfPqIk1ZelkAcEPjerE62fdPnWRRBHuZGv4+Vrk0eBgnt422PuvohTaUA98r8
13mOL9zcXRXT03VNoV2SsHFBcs2tPQ6K74O7CtB5PZWPWJVvb4HcJt0n9PMUuba5HS
14fUlnI5OB6PwLgrFiABiR+fmbskUSdcv+CY0ZT4IDYGB2/m2egzo20HKBWYwVBEO3
15T5vJv1+V
16=hJ1B
17-----END PGP SIGNATURE-----

in src/util/thread.cpp:14 in 26cf9ea8e4

 9@@ -10,10 +10,12 @@
10 
11 #include <exception>
12 #include <functional>
13+#include <string>
14+#include <utility>

hebasto commented at 7:47 pm on September 15, 2022:

nit: Why #include <utility>?

MarcoFalke commented at 8:01 pm on September 15, 2022:

std::move (Edit: Oh yeah. That one isn’t used.)

hebasto commented at 8:04 pm on September 15, 2022:

0$ git grep move -- src//util/thread.cpp | wc -l
10

MarcoFalke commented at 10:54 am on September 16, 2022:

iwyu:

 0util/thread.h should add these lines:
 1#include <string_view>  // for string_view
 2
 3util/thread.h should remove these lines:
 4- #include <string>  // lines 9-9
 5
 6The full include-list for util/thread.h:
 7#include <functional>   // for function
 8#include <string_view>  // for string_view
 9---
10
11util/thread.cpp should add these lines:
12
13util/thread.cpp should remove these lines:
14- #include <utility>  // lines 14-14
15
16The full include-list for util/thread.cpp:
17#include <util/thread.h>
18#include <logging.h>           // for LogPrintf_, LogPrintf
19#include <util/system.h>       // for PrintExceptionContinue
20#include <util/threadnames.h>  // for ThreadRename
21#include <exception>           // for exception
22#include <functional>          // for function
23#include <string>              // for allocator, string
24---

hebasto approved

hebasto commented at 7:52 pm on September 15, 2022: member

ACK 26cf9ea8e44d7fd6450336f567afaedd1275baf7, I have reviewed the code and it looks OK.

w0xlt approved

w0xlt commented at 8:39 pm on September 15, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/25971/commits/26cf9ea8e44d7fd6450336f567afaedd1275baf7

MarcoFalke merged this on Sep 16, 2022

MarcoFalke closed this on Sep 16, 2022

sidhujag referenced this in commit e32da13ac8 on Sep 20, 2022

stickies-v deleted the branch on Sep 22, 2022

bitcoin locked this on Sep 22, 2023

refactor: Use std::string for thread and index names #25971

Conflicts