Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCd91e8262c060: Add AddInt63Overflow and SubInt63Overflow

Summary

Computes 63+sign-bit addition and subtraction with overflow checks. It uses builtin_saddll_overflow and builtin_ssubll_overflow if available and falls back to AddInt63OverflowEmulated and SubInt63OverflowEmulated otherwise.

Allows 63+sign-bit integers in Script down the line.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tobias_ruck created this revision.Aug 25 2021, 04:44

Owners added a reviewer: Restricted Owners Package.Aug 25 2021, 04:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 25 2021, 04:44

Harbormaster completed remote builds in B16404: Diff 29540.Aug 25 2021, 04:44

tobias_ruck requested review of this revision.Aug 25 2021, 04:44

Fix spelling of available.

Harbormaster completed remote builds in B16405: Diff 29541.Aug 25 2021, 05:05

Fabien requested changes to this revision.Aug 30 2021, 12:13

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/config/CMakeLists.txt
59 ↗	(On Diff #29541)	You will benefit from using the default `${ARGN}` variable and `string(JOIN)` here, like: string(JOIN ", " ARGS ${ARGN}) This will let you pass any number of argument, including none.

This revision now requires changes to proceed.Aug 30 2021, 12:13

majcosta added a subscriber: majcosta.Aug 31 2021, 18:13

majcosta added inline comments.

src/script/intmath.h
39–48 ↗	(On Diff #29541)	an API that returns false when the computation is performed successfully and true when it isn't is a bit weird, you could RII"R":

Update detection of built-ins.

Harbormaster completed remote builds in B16650: Diff 29958.Sep 14 2021, 19:18

deadalnix requested changes to this revision.Sep 14 2021, 23:26

deadalnix added inline comments.

src/script/intmath.h
41 ↗	(On Diff #29958)	You need to check for the size of a long long.

This revision now requires changes to proceed.Sep 14 2021, 23:26

Add comment for the int64_t == long long int assertion.

Harbormaster completed remote builds in B16659: Diff 29975.Sep 15 2021, 16:40

Fabien accepted this revision.Sep 15 2021, 19:23

Fabien added inline comments.

src/config/CMakeLists.txt
104 ↗	(On Diff #29975)	You can make it a macro instead of a function

Ok, nevermind, the assumptions are there, but we have a file for that. This avoids baking in contradictory assumptions.

The test however needs to be improved so that correct behavior is obvious.

src/script/intmath.h
14 ↗	(On Diff #29975)	You should move this in assumption.h
src/test/intmath_tests.cpp
140 ↗	(On Diff #29975)	You should stick these checks just after the corresponding emulated check, so that it can be asserted that both return the same result easily.

This revision now requires changes to proceed.Sep 15 2021, 20:39

Verify [Add|Sub]Int63OverflowEmulated and [Add|Sub]Int63Overflow overflow for the same operands and produce the same result for for non-overflowing operands.

Harbormaster completed remote builds in B16665: Diff 29987.Sep 16 2021, 07:30

deadalnix requested changes to this revision.Sep 20 2021, 14:34

deadalnix added inline comments.

src/test/intmath_tests.cpp
86 ↗	(On Diff #29987)	This has the same problem as the previous code. You want the code to be the same for both by construct not by cutting and pasting.

This revision now requires changes to proceed.Sep 20 2021, 14:34

Add CheckArithmeticResult to intmat_tests for less copy-pasta

Harbormaster completed remote builds in B16761: Diff 30130.Sep 23 2021, 02:58

deadalnix accepted this revision.Sep 23 2021, 14:03

This revision is now accepted and ready to land.Sep 23 2021, 14:03

Closed by commit rABCd91e8262c060: Add AddInt63Overflow and SubInt63Overflow (authored by tobias_ruck). · Explain WhySep 23 2021, 15:56

This revision was automatically updated to reflect the committed changes.

tobias_ruck added a commit: rABCd91e8262c060: Add AddInt63Overflow and SubInt63Overflow.

Add AddInt63Overflow and SubInt63Overflow
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 30151

src/compat/assumptions.h

src/config/CMakeLists.txt

src/config/bitcoin-config.h.cmake.in

src/script/intmath.h

src/test/intmath_tests.cpp

Add AddInt63Overflow and SubInt63OverflowClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 30151

src/compat/assumptions.h

src/config/CMakeLists.txt

src/config/bitcoin-config.h.cmake.in

src/script/intmath.h

src/test/intmath_tests.cpp

Add AddInt63Overflow and SubInt63Overflow
ClosedPublic
Actions

Revision Contents
Changeset List