Move SECP256K1_INLINE macro definition out from include/secp256k1.h #1231

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:230131-inline changing 15 files +29 −13
  1. hebasto commented at 12:10 pm on March 9, 2023: member

    From IRC:

    06:29 < hebasto> What are reasons to define the SECP256K1_INLINE macro in user’s include/secp256k1.h header, while it is used internally only? 06:32 < hebasto> I mean, any other (or a new dedicated) header in src looks more appropriate, no? 06:35 < sipa> I think it may just predate any “utility” internal headers. 06:42 < sipa> I think it makes sense to move it to util.h

    Pros:

    • it is a step in direction to better organized headers (in context of #924, #1039)

    Cons:

    • code duplication for SECP256K1_GNUC_PREREQ macro
  2. real-or-random commented at 12:57 pm on March 9, 2023: contributor
    Oh, the CI failures revealed a bug. The examples shouldn’t use SECP256K1_INLINE.
  3. Remove `SECP256K1_INLINE` usage from examples 77445898a5
  4. hebasto force-pushed on Mar 9, 2023
  5. hebasto commented at 1:06 pm on March 9, 2023: member

    Oh, the CI failures revealed a bug. The examples shouldn’t use SECP256K1_INLINE.

    Thanks! Fixed.

  6. in src/util.h:21 in 5678ff7dd3 outdated
    16@@ -17,6 +17,27 @@
    17 #define DEBUG_CONFIG_MSG(x) "DEBUG_CONFIG: " x
    18 #define DEBUG_CONFIG_DEF(x) DEBUG_CONFIG_MSG(#x "=" STR(x))
    19 
    20+# if !defined(SECP256K1_GNUC_PREREQ)
    21+#  if defined(__GNUC__)&&defined(__GNUC_MINOR__)
    


    real-or-random commented at 1:19 pm on March 9, 2023:
    formatting differs from the copy in the public header

    hebasto commented at 1:27 pm on March 9, 2023:
    Cannot see any difference. What did I miss?

    real-or-random commented at 2:10 pm on March 9, 2023:
    Oh, you’re right… I was looking at the missing whitespace around &&, which should be added. For some reason, I believed it’s there in the public header, but I was probably looking at the wrong macro.
  7. real-or-random commented at 1:21 pm on March 9, 2023: contributor

    Concept ACK

    That’s a breaking change technically, but in this case, even I tend not to treat it as one… :see_no_evil:


    There’s no strict reason why util.h shouldn’t be able to include secp256k1.h, if it uses macro from the latter. If there’s a reason to consider that a bad idea (but I don’t think so), and we prefer code duplication, we should probably add a comment to each copy, indicating that there’s another copy that should be updated in lock-step.

  8. hebasto commented at 1:31 pm on March 9, 2023: member

    There’s no strict reason why util.h shouldn’t be able to include secp256k1.h

    Wouldn’t that be exposing of implementation details in a public interface?

  9. real-or-random commented at 2:09 pm on March 9, 2023: contributor

    There’s no strict reason why util.h shouldn’t be able to include secp256k1.h

    Wouldn’t that be exposing of implementation details in a public interface?

    I meant adding #include "../include/secp256k1.h" to util.h, and not the other way around? Or how would that expose details?

  10. hebasto commented at 3:25 pm on March 9, 2023: member

    There’s no strict reason why util.h shouldn’t be able to include secp256k1.h

    Wouldn’t that be exposing of implementation details in a public interface?

    I meant adding #include "../include/secp256k1.h" to util.h, and not the other way around? Or how would that expose details?

    You’re right. I misunderstood you.

    Well, it would work but it requires to add include/ into include directories in build systems (-I$(top_srcdir)/include in Automake, ${PROJECT_SOURCE_DIR}/include in CMake) like we do for examples now.

  11. Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` 8e142ca410
  12. hebasto force-pushed on Mar 9, 2023
  13. hebasto commented at 3:32 pm on March 9, 2023: member

    There’s no strict reason why util.h shouldn’t be able to include secp256k1.h, if it uses macro from the latter.

    Thanks! Done.

  14. hebasto cross-referenced this on Mar 9, 2023 from issue [POC] cmake: Add option to run `include-what-you-use` with compiler by hebasto
  15. real-or-random approved
  16. sipa commented at 3:52 pm on April 20, 2023: contributor
    utACK 8e142ca4102ade1b90dcb06d6c78405ef3220599
  17. real-or-random merged this on Apr 20, 2023
  18. real-or-random closed this on Apr 20, 2023

  19. hebasto deleted the branch on Apr 20, 2023
  20. sipa referenced this in commit b4eb644b6c on May 12, 2023
  21. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  22. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  23. hebasto cross-referenced this on Jun 6, 2023 from issue Drop no longer needed `#include "../include/secp256k1.h"` by hebasto
  24. real-or-random referenced this in commit 0702ecb061 on Jun 21, 2023
  25. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  26. vmta referenced this in commit 8f03457eed on Jul 1, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 17:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me