Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(math): add range check to string conversion methods #389

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

chokobole
Copy link
Contributor

@chokobole chokobole commented Apr 13, 2024

Description

This PR adds the following features:

  1. helpful error message when failing to parse BigInt<N>.
  2. add range check to PrimeFieldXXX::From(Dec|Hex)String.

@chokobole chokobole force-pushed the feat/add-range-check-to-string-conversion-methods branch from 68c2049 to 8367c09 Compare April 13, 2024 13:31
Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7d6e8c1
chore(math): add helpful error message when failed to parse BigInt
->
chore(math): add helpful error message when failing to parse BigInt

Copy link
Contributor

@Insun35 Insun35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tachyon/math/finite_fields/prime_field_generic.h Outdated Show resolved Hide resolved
tachyon/math/finite_fields/prime_field_generic.h Outdated Show resolved Hide resolved
tachyon/math/finite_fields/prime_field_gpu.h Outdated Show resolved Hide resolved
tachyon/math/finite_fields/prime_field_gpu.h Outdated Show resolved Hide resolved
tachyon/math/finite_fields/prime_field_gpu_debug.h Outdated Show resolved Hide resolved
tachyon/math/finite_fields/prime_field_gpu_debug.h Outdated Show resolved Hide resolved
@chokobole chokobole force-pushed the feat/add-range-check-to-string-conversion-methods branch from 8367c09 to 6a9ac90 Compare April 16, 2024 08:09
Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@fakedev9999 fakedev9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

While this may appear redundant in debug mode due to the existing range check
in the constructor of PrimeField, we find it acceptable since
`From(Dec|Hex)String` is not called frequently and the performance impact
in release mode is minimal.
@chokobole chokobole force-pushed the feat/add-range-check-to-string-conversion-methods branch from 6a9ac90 to 54e801e Compare April 16, 2024 10:16
@chokobole chokobole merged commit e410ca8 into main Apr 16, 2024
3 checks passed
@chokobole chokobole deleted the feat/add-range-check-to-string-conversion-methods branch April 16, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants