Page MenuHomePhabricator

[TRIVIAL] Cleanup the JNI test file
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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
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.

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
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.

This revision was automatically updated to reflect the committed changes.