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

struct EdgeFlags: Make bitflags! #759

Merged
merged 10 commits into from
Feb 29, 2024
Merged

struct EdgeFlags: Make bitflags! #759

merged 10 commits into from
Feb 29, 2024

Conversation

folkertdev
Copy link
Collaborator

No description provided.

src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@randomPoison randomPoison 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
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Sorry again for the overlap @folkertdev. In making intra_edge.rs safe and const, I did some of this already. I didn't make EdgeFlags a bitflags! yet, though, so we can use those changes once rebased and everything made const fn-compatible, since all of the uses in intra_edge.rs are now in const fns. I'll open that PR shortly.

@folkertdev
Copy link
Collaborator Author

that's fine, #762 looks really nice!

I'll build this on top of that again when it gets merged (one extra change is that & and | are not const for the bitflags, so we'll need to use the union and intersection associated functions)

@kkysen kkysen changed the title make EdgeFlags a bitflags struct EdgeFlags: Make bitflags! Feb 26, 2024
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
@kkysen
Copy link
Collaborator

kkysen commented Feb 28, 2024

@folkertdev, wanted to let you know #762 is merged now.

src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

Looks good! Just had a couple of minor questions/comments.

src/intra_edge.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

I still had some minor comments. It mostly looks very good.

src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Almost there; found two more little simplifications.

src/intra_edge.rs Outdated Show resolved Hide resolved
src/intra_edge.rs Outdated Show resolved Hide resolved
@kkysen kkysen merged commit 86d2b18 into main Feb 29, 2024
34 checks passed
@kkysen kkysen deleted the edge-flags-bitflags branch February 29, 2024 11:59
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.

3 participants