Page MenuHomePhabricator
Differential D3794

Add a message to static_assert
ClosedPublic
Actions

Authored by Fabien on Aug 2 2019, 07:37.

Details

Reviewers
deadalnix
jasonbcox
Mengerian
markblundeberg
Group Reviewers
Restricted Project
Commits
rSTAGING4e4266cec73d: Add a message to static_assert
rABC4e4266cec73d: Add a message to static_assert
Summary

static_assert with no message is a c++17 feature, and causes clang to
generate a -Wc++17-extensions warning.

Test Plan

With Clang:

make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.Aug 2 2019, 07:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 2 2019, 07:37
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Aug 2 2019, 07:37

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 2 2019, 07:43

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7032: Diff 10575.Aug 2 2019, 07:43
jasonbcox requested changes to this revision.Aug 2 2019, 17:05
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/script/interpreter.cpp
1030 ↗(On Diff #10575)

Using the affirmative in this way in an error message reads strangely. I recommend "Schnorr multisig only supports up to 31 pubkeys."

This revision now requires changes to proceed.Aug 2 2019, 17:05
Fabien updated this revision to Diff 10582.Aug 2 2019, 18:01

Improve message wording;

teamcity added a comment.Aug 2 2019, 18:01

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 2 2019, 18:03

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Harbormaster failed remote builds in B7036: Diff 10582!Aug 2 2019, 18:03
deadalnix accepted this revision.Aug 3 2019, 07:20
teamcity added a comment.Aug 3 2019, 19:42

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 3 2019, 19:48

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7036: Diff 10582.Aug 3 2019, 19:48
markblundeberg added a subscriber: markblundeberg.EditedAug 4 2019, 00:50

Note -- it only supports up to 20 pubkeys like regular multisig, rather this static assert is only here since the implementation's usage of a 32-bit number (and >> operator) is implicitly assuming < 32 keys.

Fabien added a comment.Aug 4 2019, 07:48

@markblundeberg What do you think of rewording the message with a slightly more technical content then ?:
Schnorr multisig is limited by the checkbits size to 31 pubkeys.

Mengerian added a subscriber: Mengerian.Aug 4 2019, 19:21

@Fabien @markblundeberg How about this: "Schnorr multisig checkbits implementation assumes < 32 pubkeys."

Fabien added a comment.Aug 5 2019, 07:26

@Mengerian LGTM, will update accordingly.

Fabien updated this revision to Diff 10603.Aug 5 2019, 07:31

Rebase and use a more technical and accurate message.

teamcity added a comment.Aug 5 2019, 07:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 5 2019, 07:40

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7037: Diff 10603.Aug 5 2019, 07:40
jasonbcox accepted this revision.Aug 5 2019, 15:42
This revision is now accepted and ready to land.Aug 5 2019, 15:42
Mengerian accepted this revision.Aug 5 2019, 20:14

Wording now looks good to me

markblundeberg accepted this revision.Aug 5 2019, 23:15
Closed by commit rABC4e4266cec73d: Add a message to static_assert (authored by Fabien). · Explain WhyAug 6 2019, 09:01
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC4e4266cec73d: Add a message to static_assert.
Fabien added a commit: rSTAGING4e4266cec73d: Add a message to static_assert.Sep 19 2019, 01:38

Revision Contents
Changeset List

PathSize
src/
script/
5 lines

Diff 10626

View Options

src/script/interpreter.cpp

Loading...