util: move threadinterrupt into util/ #26292

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:move_thread_interrupt_util changing 15 files +18 −16

fanquake commented at 7:42 AM on October 11, 2022: member

Alongside thread and threadnames. It's part of libbitcoin_util.
DrahtBot added the label Utils/log/libs on Oct 11, 2022
DrahtBot cross-referenced this on Oct 11, 2022 from issue refactor: move interfaces/* to common/interfaces/* by fanquake
DrahtBot commented at 5:28 PM on October 11, 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:
- #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
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.
theuni commented at 6:17 PM on October 11, 2022: member

Concept ACK. To make it more obvious to reviewers, it might be a good time to update the comments in Makefile.am to reflect what @ryanofsky said here and here.

in src/i2p.h:13 in 7b2d0b0811 outdated

   8 | @@ -9,8 +9,8 @@
   9 |  #include <fs.h>
  10 |  #include <netaddress.h>
  11 |  #include <sync.h>
  12 | -#include <threadinterrupt.h>
  13 |  #include <util/sock.h>
  14 | +#include <util/threadinterrupt.h>

Empact commented at 8:52 PM on October 11, 2022:

nit: Could replace with class CThreadInterrupt;

DrahtBot cross-referenced this on Oct 11, 2022 from issue refactor: Split util/system into exception, shell, and fs-specific files by Empact
DrahtBot cross-referenced this on Oct 12, 2022 from issue ci: Integrate `bitcoin-tidy` clang-tidy plugin by fanquake

in src/util/sock.h:9 in 7b2d0b0811 outdated

   5 | @@ -6,7 +6,7 @@
   6 |  #define BITCOIN_UTIL_SOCK_H
   7 |  
   8 |  #include <compat/compat.h>
   9 | -#include <threadinterrupt.h>
  10 | +#include <util/threadinterrupt.h>

Empact commented at 3:56 PM on October 12, 2022:

nit: Could replace with class CThreadInterrupt;

ryanofsky approved
ryanofsky commented at 5:09 PM on October 12, 2022: contributor

Code review ACK 7b2d0b0811b0faf7f09a119354a22ecfd869720b
in src/Makefile.am:944 in 7b2d0b0811 outdated
```
 940 | @@ -942,6 +941,7 @@ libbitcoinkernel_la_SOURCES = \
 941 |    util/syserror.cpp \
 942 |    util/system.cpp \
 943 |    util/thread.cpp \
 944 | +  util/threadinterrupt.cpp \
```
hebasto commented at 6:00 PM on October 12, 2022:

Is it really required in libbitcoinkernel?

I've managed to ./configure --with-experimental-kernel-lib --enable-experimental-util-chainstate && make flawlessly without this source file.

fanquake commented at 8:42 AM on October 21, 2022:

Opened #26360.
DrahtBot cross-referenced this on Oct 12, 2022 from issue refactor: Extract util/exception from util/system by Empact
fanquake cross-referenced this on Oct 14, 2022 from issue doc: consolidate library documentation to libraries.md by fanquake
fanquake commented at 9:10 AM on October 14, 2022: member

To make it more obvious to reviewers, it might be a good time to update the comments in Makefile.am to reflect what ryanofsky said #26196#pullrequestreview-1130117250 and #26196 (comment).

Rather than adding more duplicate commentary there, I've opened #26313 to drop those comments, in favour of doc/libraries.md, which is much more comprehensive.
DrahtBot cross-referenced this on Oct 17, 2022 from issue indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky
maflcko referenced this in commit ba441d493c on Oct 18, 2022
fanquake cross-referenced this on Oct 21, 2022 from issue build: remove threadinterrupt from libbitcoinkernel by fanquake
fanquake force-pushed on Oct 21, 2022
DrahtBot cross-referenced this on Oct 24, 2022 from issue build: move util/url to common/url by fanquake
ryanofsky approved
ryanofsky commented at 5:43 PM on October 31, 2022: contributor

Code review ACK bd799386882a75bfe6032a5cf53cf1a15b29b7fb
fanquake referenced this in commit c041d8f2c9 on Nov 1, 2022
util: move threadinterrupt into util b89530483d
fanquake force-pushed on Nov 1, 2022
fanquake commented at 10:16 AM on November 1, 2022: member

Rebased past #26360 and #26294.
ryanofsky approved
ryanofsky commented at 3:12 PM on November 1, 2022: contributor

Code review ACK b89530483d39f6a6a777df590b87ba2fad8c8b60. No changes since last review other than rebase
sidhujag referenced this in commit b6455439bc on Nov 1, 2022
theuni approved
theuni commented at 8:17 PM on November 2, 2022: member

ACK b89530483d39f6a6a777df590b87ba2fad8c8b60.

No objection, but why the added includes (i2p.cpp for ex) where the matching header already pulls it in?
fanquake merged this on Nov 22, 2022
fanquake closed this on Nov 22, 2022
fanquake deleted the branch on Nov 22, 2022
sidhujag referenced this in commit 616f8589d2 on Nov 22, 2022
Pythonix cross-referenced this on Dec 6, 2022 from issue util: move threadinterrupt.{cpp,h} to util by Pythonix
bitcoin locked this on Nov 22, 2023

Contributors

Labels

Linked (view graph)

#26196 kernel: move RunCommandParseJSON to its own file #26313 doc: consolidate library documentation to libraries.md #26360 build: remove threadinterrupt from libbitcoinkernel

util: move threadinterrupt into util/ #26292

Conflicts