refactor: Remove unused boost signals2 from torcontrol #28240
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
DrahtBot
changed the title
No need for saturating behavior when the int is composed of 3 digits.
maflcko
force-pushed
the
2308-remove-boost-001-
branch
from
0000029
to
fa32af2
Compare
There was a problem hiding this comment.
ACK fa32af2
The async_handler was unused since its introduction, so I don't think it's worth it to keep it around.
src/compat/compat.h
Outdated
| #include <netinet/tcp.h> | ||
| #include <arpa/inet.h> | ||
| #include <ifaddrs.h> | ||
| #include <fcntl.h>// IWYU pragma: export |
There was a problem hiding this comment.
Style nit: These pragmas could be indented like they are here: https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L20 , or did you omit any automatic formatting to maintain order?
There was a problem hiding this comment.
I wasn't sure if the includes must come in a specific order. I've pushed a new commit to re-order them, which can be easily reverted, if there are issues.
| self->message.lines.push_back(s.substr(4)); | ||
| char ch = s[3]; // '-','+' or ' ' | ||
| if (ch == ' ') { | ||
| // Final line, dispatch reply and clean up | ||
| if (self->message.code >= 600) { | ||
| // (currently unused) | ||
| // Dispatch async notifications to async handler |
There was a problem hiding this comment.
Maybe change this to "These messages could be used to dispatch async notifications to an async handler"?
Can be reviewed with: --color-moved=blocks --color-moved-ws=ignore-all-space --ignore-all-space
|
Re-ACK faaba77 |
|
|
||
| #include <event2/bufferevent.h> | ||
| #include <event2/event.h> |
There was a problem hiding this comment.
Is this (another) a bug in IWYU? Seems a bit weird to remove the <event2/event.h> include, which, as far as I can tell, is correct. It's where event_base is defined, and what is expected when using libevent:
section usage Standard usage
Every program that uses Libevent must include the <event2/event.h>
header, and pass the -levent flag to the linker.
There was a problem hiding this comment.
I think the since the libevent structs in this file are only pointers, the actual definitions aren't needed hence the include is not strictly necessary? It still compiles at least.
There was a problem hiding this comment.
Yeah, iirc everything in libevent can be forward-declared except for evutil_socket_t. It's very annoying.
Because everything else is already forward-declared as @achow101 mentioned, only the definition of evutil_socket_t is needed. evutil_socket_t is actually defined in event2/util.h (note that event.h includes util.h.
So I agree with this change. It's a small reduction in needed includes.
|
ACK faaba77 |
…ontrol faaba77 Sort includes in compat.h (MarcoFalke) fa91a23 remove unused limits.h include in compat.h (MarcoFalke) fa32af2 Replace LocaleIndependentAtoi with ToIntegral (MarcoFalke) faab76c iwyu on torcontrol (MarcoFalke) fa0a60d Remove unused boost signals2 from torcontrol (MarcoFalke) Pull request description: Remove unused boost, and other includes, and other legacy functions from torcontrol. ACKs for top commit: TheCharlatan: Re-ACK faaba77 achow101: ACK faaba77 dergoegge: utACK faaba77 Tree-SHA512: 440f8d3ae9c3cf4dcc368e35b29459b5fcec8c6d233e8f9be3a854e7624b8633d6ccdde10cb0c6f74f86278e06557c4e9e24de30c3c692826237939265c6160a
Remove unused boost, and other includes, and other legacy functions from torcontrol.