Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: move threadinterrupt into util/ #26292

Merged
merged 1 commit into from
Nov 22, 2022
Merged

util: move threadinterrupt into util/ #26292

merged 1 commit into from
Nov 22, 2022

Conversation

fanquake
Copy link
Member

Alongside thread and threadnames. It's part of libbitcoin_util.

@DrahtBot DrahtBot mentioned this pull request Oct 11, 2022
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 11, 2022 •
edited
Loading

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
Copy link
Member

theuni commented Oct 11, 2022

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.

Empact
Empact reviewed Oct 11, 2022
View reviewed changes
src/i2p.h
#include <util/sock.h>
#include <util/threadinterrupt.h>
Copy link
Contributor

@Empact Empact Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could replace with class CThreadInterrupt;

This was referenced Oct 11, 2022
Closed
Merged
Empact
Empact reviewed Oct 12, 2022
View reviewed changes
src/util/sock.h
@@ -6,7 +6,7 @@
#define BITCOIN_UTIL_SOCK_H

#include <compat/compat.h>
#include <threadinterrupt.h>
#include <util/threadinterrupt.h>
Copy link
Contributor

@Empact Empact Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could replace with class CThreadInterrupt;

ryanofsky
ryanofsky approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 7b2d0b0

hebasto
hebasto reviewed Oct 12, 2022
View reviewed changes
src/Makefile.am Outdated Show resolved Hide resolved
@DrahtBot DrahtBot mentioned this pull request Oct 12, 2022
Closed
@fanquake fanquake mentioned this pull request Oct 14, 2022
Merged
@fanquake
Copy link
Member Author

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 (review) 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 DrahtBot mentioned this pull request Oct 17, 2022
Draft
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Oct 18, 2022
Merge bitcoin/bitcoin#26313: doc: consolidate library documentation t…
ba441d4 
…o libraries.md

af781bf doc: fix typo in doc/libraries.md (fanquake)
9e9ae61 doc: remove library commentary from src/Makefile.am (fanquake)

Pull request description:

  Deduplicate the makefile comments, in favour of doc/libraries.md. I think a single, more comprehensive source of truth is preferable. Diagrams are also useful. Came up in bitcoin/bitcoin#26292 (comment).

ACKs for top commit:
  ryanofsky:
    Code review ACK af781bf, nice cleanups
  hebasto:
    ACK af781bf, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: df61ed1394102221701ae2dfa42886dfabe9d9fd7f601b794e2195f93d8f7c2a1cd1c000a77d0a969b42328e8ebc0387755c57291837b283fdf376dbd98fdda1
@fanquake fanquake mentioned this pull request Oct 21, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Oct 24, 2022
Closed
ryanofsky
ryanofsky approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK bd79938

fanquake added a commit that referenced this pull request Nov 1, 2022
@fanquake
Merge #26360: build: remove threadinterrupt from libbitcoinkernel
c041d8f 
afbcd22 build: remove threadinterrupt from libbitcoinkernel (fanquake)

Pull request description:

  Extracted from #26292.

ACKs for top commit:
  hebasto:
    ACK afbcd22, tested on Ubuntu 22.04.
  ryanofsky:
    Code review ACK afbcd22

Tree-SHA512: 9d355f0e417561be41cdd0674a8f94c9ffe3ecfb4063bb9c90f1032cb9d471be11d4fa26de40993e3a411e015272201551fbbb3d3c2b43e4c17bf49386a2741c
@fanquake
Copy link
Member Author

fanquake commented Nov 1, 2022

Rebased past #26360 and #26294.

ryanofsky
ryanofsky approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK b895304. No changes since last review other than rebase

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 1, 2022
@fanquake
Merge bitcoin#26360: build: remove threadinterrupt from libbitcoinkernel
b645543 
afbcd22 build: remove threadinterrupt from libbitcoinkernel (fanquake)

Pull request description:

  Extracted from bitcoin#26292.

ACKs for top commit:
  hebasto:
    ACK afbcd22, tested on Ubuntu 22.04.
  ryanofsky:
    Code review ACK afbcd22

Tree-SHA512: 9d355f0e417561be41cdd0674a8f94c9ffe3ecfb4063bb9c90f1032cb9d471be11d4fa26de40993e3a411e015272201551fbbb3d3c2b43e4c17bf49386a2741c
theuni
theuni approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b895304.

No objection, but why the added includes (i2p.cpp for ex) where the matching header already pulls it in?

@fanquake fanquake merged commit 1b68094 into bitcoin:master Nov 22, 2022
@fanquake fanquake deleted the move_thread_interrupt_util branch November 22, 2022 09:53
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 22, 2022
@fanquake
Merge bitcoin#26292: util: move threadinterrupt into util/
616f858
@Pythonix Pythonix mentioned this pull request Dec 6, 2022
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto left review comments

@Empact Empact Empact left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

@theuni theuni theuni approved these changes

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fanquake @DrahtBot @theuni @Empact @ryanofsky @hebasto