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

Added BitNot operation. #3021

Merged
merged 1 commit into from
May 3, 2023
Merged

Added BitNot operation. #3021

merged 1 commit into from
May 3, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented May 3, 2023

This change is Reviewable

@orizi orizi requested a review from spapinistarkware May 3, 2023 10:00
@orizi orizi linked an issue May 3, 2023 that may be closed by this pull request
Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 14 files at r1, all commit messages.
Reviewable status: 5 of 14 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-parser/src/lexer.rs line 376 at r1 (raw file):

    LT,
    Not,
    Tilde,

Our token are not named bang, dash, etc... They are baned Not, Minus, after the math operation. If we wanna be consistent, this should be BitNot or BitwiseNot.

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 9 of 14 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

Copy link
Collaborator Author

@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: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


crates/cairo-lang-parser/src/lexer.rs line 376 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Our token are not named bang, dash, etc... They are baned Not, Minus, after the math operation. If we wanna be consistent, this should be BitNot or BitwiseNot.

i'm not really sure what is better in this case - do you really think BitNot is clearer?

@spapinistarkware
Copy link
Contributor

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

crates/cairo-lang-parser/src/lexer.rs line 376 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…
i'm not really sure what is better in this case - do you really think BitNot is clearer?

I dunno, but I'd rather be consistent

@orizi orizi force-pushed the orizi/added-tilde-op branch from f8dd784 to 9c0c399 Compare May 3, 2023 11:01
Copy link
Collaborator Author

@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: 4 of 14 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


crates/cairo-lang-parser/src/lexer.rs line 376 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I dunno, but I'd rather be consistent

Done.

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi added this pull request to the merge queue May 3, 2023
Merged via the queue into main with commit c1db0f8 May 3, 2023
@orizi orizi deleted the orizi/added-tilde-op branch May 9, 2023 12:35
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.

feat: bitwise not
2 participants