Page MenuHomePhabricator
Differential D3834

[TRIVIAL] Cleanup the JNI test file
ClosedPublic
Actions

Authored by Fabien on Aug 9 2019, 10:38.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file
rABC0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file
Summary

Cleanup the NativeSecp256k1Test.java file:

  • Move comments on their own lines;
  • Add period at the end of sentences;
  • Remove extra whitespaces in parenthesis to make the code consistent;
  • Add/Remove some newlines where needed;
  • Remove commented out debug code;
  • Replace tabs with spaces;
  • ...
Test Plan
./contrib/teamcity/build-secp256k1.sh

Diff Detail

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

Event Timeline

Fabien created this revision.Aug 9 2019, 10:38
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 9 2019, 10:38
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Aug 9 2019, 10:38

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added a child revision: D3835: [SECP256K1] JNI tests : remove dependency to obsolete DatatypeConverter.Aug 9 2019, 10:46
teamcity added a comment.Aug 9 2019, 10:49

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7093: Diff 10695.Aug 9 2019, 10:49
jasonbcox requested changes to this revision.Aug 9 2019, 15:45
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
13 ↗(On Diff #10695)

mixed comment types

196 ↗(On Diff #10695)

Kind of weird to append a period on the end of a link. maybe do this instead?

* It tests the following test vectors:
* https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr/test-vectors.csv
This revision now requires changes to proceed.Aug 9 2019, 15:45
Fabien updated this revision to Diff 10725.Aug 12 2019, 12:53

Update as per comment.
Add a newline between the TODO and the javadoc comments for better readability.

teamcity added a comment.Aug 12 2019, 12:54

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7114: Diff 10725.Aug 12 2019, 13:02
teamcity added a comment.Aug 12 2019, 13:02

Build Bitcoin-ABC / Diffs / Diff Testing passed.

deadalnix added a comment.Aug 13 2019, 06:03

Are you really confident you want to take ownership of this?

Fabien added a comment.Aug 13 2019, 06:48

@deadalnix It's already the case, see D2681 or D2755. Also I have D3835 that will take ownership even more, but make your life easier for testing with buster.

jasonbcox accepted this revision.Aug 13 2019, 21:38
This revision is now accepted and ready to land.Aug 13 2019, 21:38
deadalnix added a comment.Aug 14 2019, 05:02
In D3834#90773, @Fabien wrote:

@deadalnix It's already the case, see D2681 or D2755. Also I have D3835 that will take ownership even more, but make your life easier for testing with buster.

D3835 clearly have happened one way or another already: https://github.com/bitcoin-core/secp256k1/blob/master/src/java/org/bitcoin/NativeSecp256k1Test.java

Fabien added a comment.Aug 14 2019, 07:39

@deadalnix Absolutely, sepc256k1 relies on BaseEncoding from the guava dependency which was removed from our codebase by D2681, hence the custom function in D3835.

Closed by commit rABC0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file (authored by Fabien). · Explain WhySep 9 2019, 12:32
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file.
Fabien added a commit: rSTAGING0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file.Sep 19 2019, 02:12

Revision Contents
Changeset List

PathSize
src/
secp256k1/
src/
java/
org/
bitcoin/
155 lines

Diff 11158

View Options

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java

Loading...