Drop unused arith_uint256 ! operator #13396

1. 
   Empact commented at 9:17 AM on June 5, 2018: member

   All the other operators are integer or bitwise operations, and this is unused apart from tests.

   Note attempting to call ! on arith_uint256 results in a build error after this change:

   test/arith_uint256_tests.cpp:201:17: error: invalid argument type 'const arith_uint256' to unary expression
       BOOST_CHECK(!ZeroL);
   

2. Drop uint 256 not operator

   All the other operators are integer or bit operations, and this is unused
   apart from tests.

   2acd1d6716
3. fanquake added the label Refactoring on Jun 5, 2018
4. fanquake requested review from sipa on Jun 5, 2018
5. Empact renamed this:
   Drop unused uint 256 not operator
   Drop unused arith_uint256 ! operator
   on Jun 5, 2018
6. 
   promag commented at 11:52 AM on June 5, 2018: member

   utACK 2acd1d6.

7. 
   laanwj commented at 1:11 PM on June 5, 2018: member

   Isn't ! expected functionality for bigint arithmetic? I'm aware that we're not using it at the moment, but I'd like to invoke the principle of least surprise here

8. 
   skeees commented at 3:47 PM on June 5, 2018: contributor

   Agree with @laanwj @Empact - is the only benefit here to reduce binary size? I don't know enough about the linker to determine whether it will include this code even if currently unused

9. 
   MarcoFalke commented at 3:54 PM on June 5, 2018: member

   When was it last used?

10. 
    laanwj commented at 4:34 PM on June 5, 2018: member

    It's a function in the include file, so I would even doubt this affect the binary size. Also ti's properly tested so that can't be an argument to remove it either. I'd say NACK, sorry.

11. 
    DrahtBot commented at 5:24 PM on June 5, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #13388 (util: Implement boolean conversion and !operator for uint* by qmma70)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

12. 
    Empact commented at 7:40 PM on June 5, 2018: member

    My motivation is to reduce lines of code because I view unused functionality as a tax on readers of the code, even if those lines don't impact the binaries. I guess you could say this is partly an expression of the YAGNI principle.

13. 
    qmma70 commented at 12:05 AM on June 6, 2018: contributor

    I opened a related PR #13388, if anyone's interested.

14. 
    laanwj commented at 5:11 AM on June 7, 2018: member

    Oke, forget my argument about API symmetry. There is no operator bool() on arith_uint256, so why would bool operator!() exist. This means you can do if (!a) but not if(a). Hmm.

    utACK 2acd1d6716959b99e751cf85a7c47aaa383e937f

15. 
    practicalswift commented at 5:12 PM on June 7, 2018: contributor

    utACK 2acd1d6716959b99e751cf85a7c47aaa383e937f

16. laanwj merged this on Jun 7, 2018
17. laanwj closed this on Jun 7, 2018

    ---

18. laanwj referenced this in commit 97073f8837 on Jun 7, 2018
19. Empact deleted the branch on Jun 8, 2018
20. UdjinM6 referenced this in commit 667f90498e on Jun 19, 2021
21. UdjinM6 referenced this in commit d8264437d7 on Jun 24, 2021
22. UdjinM6 referenced this in commit b4a4823900 on Jun 26, 2021
23. UdjinM6 referenced this in commit 59e10d97a4 on Jun 26, 2021
24. UdjinM6 referenced this in commit 52d2042289 on Jun 26, 2021
25. PastaPastaPasta referenced this in commit d01959b077 on Jun 27, 2021
26. PastaPastaPasta referenced this in commit 9affcf2071 on Jun 28, 2021
27. PastaPastaPasta referenced this in commit 979f8afa44 on Jun 28, 2021
28. UdjinM6 referenced this in commit 26a56f2b03 on Jun 28, 2021
29. PastaPastaPasta referenced this in commit fa6793a96a on Jun 29, 2021
30. MarcoFalke locked this on Sep 8, 2021

Contributors

EmpactpromaglaanwjskeeesMarcoFalkeDrahtBotqmma70practicalswift

Review Requested
sipa