refactor: Remove unused boost signals2 from torcontrol #28240

1. 
   maflcko commented at 12:56 pm on August 8, 2023: member

   Remove unused boost, and other includes, and other legacy functions from torcontrol.
2. Remove unused boost signals2 from torcontrol fa0a60dd93
3. 
   DrahtBot commented at 12:56 pm on August 8, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, dergoegge, achow101

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #28101 (init: changing -torcontrol help to specify that a default port is used by kevkevinpal)
   • #27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)

   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.

4. DrahtBot renamed this:
   refactor: Remove unused boost signals2 from torcontrol
   refactor: Remove unused boost signals2 from torcontrol
   on Aug 8, 2023
5. DrahtBot added the label Refactoring on Aug 8, 2023
6. iwyu on torcontrol faab76c1c0
7. Replace LocaleIndependentAtoi with ToIntegral

   No need for saturating behavior when the int is composed of 3 digits.

   fa32af22b3
8. maflcko force-pushed on Aug 8, 2023
9. in src/compat/compat.h:25 in faab76c1c0 outdated

   30-#include <net/if.h>
   31-#include <netinet/in.h>
   32-#include <netinet/tcp.h>
   33-#include <arpa/inet.h>
   34-#include <ifaddrs.h>
   35+#include <fcntl.h>// IWYU pragma: export
   

   ---

   ---

   

   TheCharlatan commented at 2:50 pm on August 8, 2023:

   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?

   ---

   

   maflcko commented at 3:52 pm on August 8, 2023:

   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.
10. in src/torcontrol.cpp:101 in fa32af22b3 outdated

     97         char ch = s[3]; // '-','+' or ' '
     98         if (ch == ' ') {
     99             // Final line, dispatch reply and clean up
    100             if (self->message.code >= 600) {
    101+                // (currently unused)
    102                 // Dispatch async notifications to async handler
    

    ---

    ---

    

    TheCharlatan commented at 3:31 pm on August 8, 2023:

    Maybe change this to “These messages could be used to dispatch async notifications to an async handler”?
11. TheCharlatan approved
12. 
    TheCharlatan commented at 3:32 pm on August 8, 2023: contributor

    ACK fa32af22b323e7c58b6b20af6517f4795a72cdc5

    The async_handler was unused since its introduction, so I don’t think it’s worth it to keep it around.

13. remove unused limits.h include in compat.h fa91a23d63
14. Sort includes in compat.h

    Can be reviewed with:
    --color-moved=blocks  --color-moved-ws=ignore-all-space --ignore-all-space

    faaba770e1
15. 
    TheCharlatan commented at 4:59 pm on August 8, 2023: contributor

    Re-ACK faaba770e11352ddf6414b9855f4baa46a967580
16. dergoegge approved
17. 
    dergoegge commented at 9:53 am on August 9, 2023: member

    utACK faaba770e11352ddf6414b9855f4baa46a967580
18. maflcko requested review from fanquake on Aug 15, 2023
19. in src/torcontrol.h:16 in faab76c1c0 outdated

    10@@ -11,18 +11,14 @@
    11 #include <netaddress.h>
    12 #include <util/fs.h>
    13 
    14+#include <event2/util.h>
    15 
    16-#include <event2/bufferevent.h>
    17-#include <event2/event.h>
    

    ---

    ---

    

    fanquake commented at 10:38 am on August 15, 2023:

    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.

    ---

    

    achow101 commented at 7:40 pm on August 15, 2023:

    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.

    ---

    

    theuni commented at 7:55 pm on August 15, 2023:

    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.

20. 
    achow101 commented at 9:09 pm on August 15, 2023: member

    ACK faaba770e11352ddf6414b9855f4baa46a967580
21. achow101 merged this on Aug 15, 2023
22. achow101 closed this on Aug 15, 2023

    ---

23. maflcko deleted the branch on Aug 15, 2023
24. 
    atou69 commented at 5:49 am on August 16, 2023: none

    #27511
25. sidhujag referenced this in commit 90ee2ba0a5 on Aug 17, 2023
26. bitcoin locked this on Aug 15, 2024

Contributors
maflcko DrahtBot TheCharlatan dergoegge fanquake achow101 theuni atou69

Labels
Refactoring