Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Rename Word::zero to Word::zero_expr #1748

Closed
wants to merge 5 commits into from
Closed

Rename Word::zero to Word::zero_expr #1748

wants to merge 5 commits into from

Conversation

ChihChengLiang
Copy link
Collaborator

Description

Address #1746 (comment), we rename the constructors for zero word and one word.

Type of change

  • Refactor

Contents

  • Rename Word<Expression>::zero to zero_expr
  • Rename Word<Expression>::one to one_expr
  • Rename Word::zero_f to one
  • Rename Word::one_f to one

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Feb 1, 2024
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Another thoughts: how about define a trait with one(), zero() and let implement it for for some basic types ? Idea is to keep succinct Word::one() Word:zero() and let type system infer for us

Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@KimiWu123
Copy link
Contributor

Another thoughts: how about define a trait with one(), zero() and let implement it for for some basic types ? Idea is to keep succinct Word::one() Word:zero() and let type system infer for us

I tried to to have a trait for zero() and one(), but the compiler complained something like one() has been implemented.... Probably, there are some tricks to fix the error but I'm not familiar with it. Maybe you could give it a try!

@hero78119
Copy link
Member

Another thoughts: how about define a trait with one(), zero() and let implement it for for some basic types ? Idea is to keep succinct Word::one() Word:zero() and let type system infer for us

I tried to to have a trait for zero() and one(), but the compiler complained something like one() has been implemented.... Probably, there are some tricks to fix the error but I'm not familiar with it. Maybe you could give it a try!

I tried to implement it in another PR and seems to work #1754

If new PR is acceptable I think we can close this one.

@ChihChengLiang ChihChengLiang mentioned this pull request Feb 5, 2024
@ChihChengLiang
Copy link
Collaborator Author

@hero78119 Nice approach on #1754. I built on top of #1754 to get #1758, which removes the need to type hint the word.

@hero78119 @KimiWu123 Can you share your opinion on #1758?

@hero78119
Copy link
Member

hero78119 commented Feb 6, 2024

@hero78119 Nice approach on #1754. I built on top of #1754 to get #1758, which removes the need to type hint the word.

@hero78119 @KimiWu123 Can you share your opinion on #1758?

Just different topic

For unifying compress_f() to compress(), due to it got different implementation on different generic type, so it's can't be resolve like the same way. We might require feature like

Maybe we keep compress_f() first and left it to future work

@ChihChengLiang
Copy link
Collaborator Author

Close in favor of #1758

github-merge-queue bot pushed a commit that referenced this pull request Feb 6, 2024
### Description
to address discussion in PR
#1748

### Rationale

implement OpsIdentity for word<T> so that we no longer need to type hint
`::<Expression<F>>` or `::<F>` every time.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants