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: (try) Support u384 numeric literals #6146

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

feltroidprime
Copy link

@feltroidprime feltroidprime commented Aug 6, 2024

u384 takes 4-5 lines to write as constant or in tests, so this would improve a lot readability and dev experience

Currently i've added

  • the trait implementation in cairo in circuits.cairo
  • a simple test in circuit_test.cairo
  • a branch in the function validate_literal in crates/cairo-lang-semantic/src/corelib.rs for the type
  • a branch in the function value_as_const_value in crates/cairo-lang-semantic/src/items/constant.rs to split the limbs for the type

However when running ./scripts/cairo_test.sh the issue is that get_core_ty_by_name(db, "u384".into(), vec![]) says that u384 doesn't exist (Unknown type)

Would appreciate some help from this point. Thank you!

This change is Reviewable

@feltroidprime feltroidprime marked this pull request as draft August 6, 2024 00:53
@feltroidprime feltroidprime marked this pull request as ready for review August 6, 2024 01:12
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @feltroidprime)

a discussion (no related file):
i don't think we should - u384 is a special type for circuits - i don't think people should really use it out of the circuits scope.
having a literal for it makes it "overly friendly"


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.

2 participants