refactor: Remove stray cs_main redundant declaration #26965

1. 
   maflcko commented at 8:36 am on January 25, 2023: member

   Looks like this was forgotten when introducing kernel/cs_main ?

   Also, there is a commit to export threadsafety.h from sync.h.

2. doc: Export threadsafety.h from sync.h

   All places that include sync.h will likely need threadsafety
   annotations, so export them.

   fa02591edf
3. 
   DrahtBot commented at 8:36 am on January 25, 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	hebasto

   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:

   • #26705 (clang-tidy: Check headers and fixes them by hebasto)
   • #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es by vasild)

   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 stray cs_main redundant declaration
   refactor: Remove stray cs_main redundant declaration
   on Jan 25, 2023
5. DrahtBot added the label Refactoring on Jan 25, 2023
6. refactor: Remove stray cs_main redundant declaration faba08b5b4
7. maflcko force-pushed on Jan 25, 2023
8. 
   hebasto commented at 9:02 am on January 25, 2023: member

   Concept ACK.
9. hebasto approved
10. 
    hebasto commented at 3:42 pm on January 25, 2023: member

    ACK faba08b5b4dd5dedb457db18696de6e15839c696

    Also, there is a commit to export threadsafety.h from sync.h.

    I’m not sure about that because thread safety annotations are mostly used in headers, and sync.h can be needed to be included in source files only. For example, without that commit, node/utxo_snapshot.h needs a replacement s/#include <sync.h>/#include <threadsafety.h>/.

11. 
    maflcko commented at 3:58 pm on January 25, 2023: member

    sync.h can be needed to be included in source files only

    No? If the header uses a Mutex, it will need to include sync.h. Is there any place in the codebase that doesn’t use Mutex?

12. 
    hebasto commented at 4:03 pm on January 25, 2023: member

    Is there any place in the codebase that doesn’t use Mutex?

    For example, node/utxo_snapshot.h from this PR?

13. 
    maflcko commented at 7:51 am on January 26, 2023: member

    Well, I meant Mutex or RecursiveMutex, or whatever else is in sync.h.

    The point is that you can’t compile utxo_snapshot.h without sync.h, and any other place that uses symbols with theadsafety annotations from sync.h, in which case including theadsafety.h again serves no purpose other than noise.

14. 
    hebasto commented at 8:10 am on January 26, 2023: member

    Well, I meant Mutex or RecursiveMutex, or whatever else is in sync.h.

    The point is that you can’t compile utxo_snapshot.h without sync.h

    It is job of <kernel/cs_main.h> now, no?

    and any other place that uses symbols with theadsafety annotations from sync.h, in which case including theadsafety.h again serves no purpose other than noise.

    Agree. Probably the use case of <kernel/cs_main.h> is the only counter example.

15. 
    maflcko commented at 8:23 am on January 26, 2023: member

    I think the only counter example (something that doesn’t need sync.h directly or indirectly) is logging.h, because it only needs threadsafety, see https://github.com/bitcoin/bitcoin/blob/d4c180ecc9aec5baa6c074f78e4d2c5e28b124ac/src/logging.h#L10

    Though, I am not changing that in this pull.

16. 
    maflcko commented at 11:57 am on January 30, 2023: member

    cc @fanquake
17. fanquake merged this on Jan 30, 2023
18. fanquake closed this on Jan 30, 2023

    ---

19. sidhujag referenced this in commit dc2b0c29e5 on Jan 30, 2023
20. maflcko deleted the branch on Jan 31, 2023
21. bitcoin locked this on Jan 31, 2024

Contributors
maflcko DrahtBot hebasto

Labels
Refactoring