build: Remove unused visibility checks #30590

1. 
   fanquake commented at 2:11 pm on August 5, 2024: member

   These are unused (since libbitcoinconsensus / #29648), and the current CMake port doesn’t quite match behaviour, such that there’s no real point in doing the check. So rather than port anything, just remove it. If these are needed again in future (i.e for kernel or similar), they can be revisted, and it might be the case that build-system level checks will not be wanted.
2. build: remove check for __attribute__((visibility.. 37c9abdc43
3. build: remove check for __declspec(dllexport) bbcba09cd5
4. 
   DrahtBot commented at 2:11 pm on August 5, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	hebasto, TheCharlatan, willcl-ark
   Concept ACK	theuni

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

5. DrahtBot added the label Build system on Aug 5, 2024
6. hebasto added the label Needs CMake port on Aug 5, 2024
7. 
   hebasto commented at 2:13 pm on August 5, 2024: member

   Concept ACK on dropping unused staff.
8. 
   theuni commented at 4:07 pm on August 5, 2024: member

   Concept ACK.

   We’ll definitely want these back for the kernel at some point, yes. Though, sure, we can re-add them in a more modern way when the time comes.

9. hebasto approved
10. 
    hebasto commented at 4:27 pm on August 5, 2024: member

    ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576. I’ve verified that neither HAVE_DEFAULT_VISIBILITY_ATTRIBUTE nor HAVE_DLLEXPORT_ATTRIBUTE are used or evaluated in the current codebase.
11. DrahtBot requested review from theuni on Aug 5, 2024
12. TheCharlatan approved
13. 
    TheCharlatan commented at 12:48 pm on August 6, 2024: contributor

    ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576

    The visibility("default") attribute is still used in src/tinyformat.h, but I don’t think we need this check for that, since it is already gated.

14. 
    fanquake commented at 12:57 pm on August 6, 2024: member

    but I don’t think we need this check for that, since it is already gated.

    Yea. I think for our use, that (and related) code could also just be removed entirely.

15. willcl-ark approved
16. 
    willcl-ark commented at 3:28 pm on August 6, 2024: member

    ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576

    Makes sense to remove this while it’s unused.

    I did find a usage of HAVE_DEFAULT_VISIBILITY_ATTRIBUTE in my source dir, but it’s only present because make distclean does not remove the file src/config/bitcoin-config.h.in (which I don’t think is worth tidying with a new build system on the way)

17. fanquake merged this on Aug 6, 2024
18. fanquake closed this on Aug 6, 2024

    ---

19. fanquake deleted the branch on Aug 6, 2024
20. 
    hebasto commented at 4:51 pm on August 6, 2024: member

    Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/307.
21. hebasto removed the label Needs CMake port on Aug 6, 2024
22. hebasto referenced this in commit 3c740846bf on Aug 6, 2024

Contributors
fanquake DrahtBot hebasto theuni TheCharlatan willcl-ark

Review Requested
theuni

Labels
Build system