-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Bump GCC_SNAPSHOT_MAJOR to 15 #1583
Conversation
GCC 15 has a new warning, see https://gcc.gnu.org/pipermail/gcc-patches/2024-June/656014.html . |
Yeah, I guess one solution would be to be verbose and write the array: static const unsigned char PREFIX[19] = {'s', 'e', 'c', 'p', '2', '5', '6', 'k', '1', ' ', 't', 'e', 's', 't', ' ', 'i', 'n', 'i', 't'}; an alternative would be to disable the warning, which seems cleaner and any issues that the warning theoretically could uncover should be detectable by a the existing CI sanitizer builds? |
This is ultimately a question of taste, but I suggest this because I think it's the most readable: static const unsigned char PREFIX[] = {'s', 'e', 'c', 'p', '2', '5', '6', 'k', '1', ' ', 't', 'e', 's', 't', ' ', 'i', 'n', 'i', 't'}; This makes it unambiguously clear that we consider it an array and not a string (i.e., it's not null-terminated), the reader doesn't need to count the chars, and we can use We define such constant arrays in multiple code locations, see |
The previous code is correct and harmless to initialize an array with a non-terminated character sequence using a string literal. However, it requires exactly specifying the array size, which can be cumbersome. Also, GCC-15 may issue the -Wunterminated-string-initialization warning. [1] Fix both issues by using array initialization. This refactoring commit does not change behavior. [1] Example warning: src/modules/schnorrsig/main_impl.h:48:46: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization] 48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce"; | ^~~~~~~~~~~~~~~
Good point about the review overhead and ambiguity. I pushed a commit with your suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa67b67, I have reviewed the code and it looks OK.
I can see gcc version 15.0.0 20240811 (experimental) (GCC)
in the CI logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK fa67b67
2f2ccc4695 Merge bitcoin-core/secp256k1#1600: cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 421ed1b46f cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 1988855079 Merge bitcoin-core/secp256k1#1586: fix: remove duplicate 'the' from header file comment b307614401 Merge bitcoin-core/secp256k1#1583: ci: Bump GCC_SNAPSHOT_MAJOR to 15 fa67b6752d refactor: Use array initialization for unterminated strings 9b0f37bff1 fix: remove duplicate 'the' from header file comment e34b476730 ci: Bump GCC_SNAPSHOT_MAJOR to 15 3fdf146bad Merge bitcoin-core/secp256k1#1578: ci: Silent Homebrew's noisy reinstall warnings f8c1b0e0e6 Merge bitcoin-core/secp256k1#1577: release cleanup: bump version after 0.5.1 7057d3c9af ci: Silent Homebrew's noisy reinstall warnings c3e40d75db release cleanup: bump version after 0.5.1 git-subtree-dir: src/secp256k1 git-subtree-split: 2f2ccc469540fde6495959cec061e95aab033148
6115628 Squashed 'src/secp256k1/' changes from 642c885b61..2f2ccc4695 (Hennadii Stepanov) Pull request description: This PR updates the libsecp256k1 subtree to bitcoin-core/secp256k1@2f2ccc4, which includes the following changes: - bitcoin-core/secp256k1#1577 - bitcoin-core/secp256k1#1578 - bitcoin-core/secp256k1#1583 - bitcoin-core/secp256k1#1586 - bitcoin-core/secp256k1#1600 The latter is required for #30791. ACKs for top commit: l0rinc: utACK ff54395 real-or-random: utACK ff54395 no blockers from libsecp256k1 side, and these commits touch only build/docs/tests and are thus particularly harmless fanquake: ACK ff54395 Tree-SHA512: 77cf1ba9aa24efdcf01e99850ea8bed54f847307a3c98c10289c9b7fd9e70c9219f28bee0f00ef7d69979d95a0ddac1e937d3d183ebc9d9b8e6cce0db1d507c9
4c57c7a5a Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8e Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 2f2ccc469 Merge bitcoin-core/secp256k1#1600: cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 421ed1b46 cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 198885507 Merge bitcoin-core/secp256k1#1586: fix: remove duplicate 'the' from header file comment b30761440 Merge bitcoin-core/secp256k1#1583: ci: Bump GCC_SNAPSHOT_MAJOR to 15 fa67b6752 refactor: Use array initialization for unterminated strings 9b0f37bff fix: remove duplicate 'the' from header file comment e34b47673 ci: Bump GCC_SNAPSHOT_MAJOR to 15 7c987ec89 cmake: Call `enable_testing()` unconditionally 6aa576515 cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: 4c57c7a5a9531874e965379119621f1ab500f2fe
Follow-up to #1313
Clang should silently follow the
main
devel branch, but GCC needs to be bumped manually.