Skip to content

fidget::context::Context: add if_nonzero_else fuction#64

Merged
mkeeter merged 1 commit intomkeeter:mainfrom
Wulfsta:if_nonzero_else
Apr 8, 2024
Merged

fidget::context::Context: add if_nonzero_else fuction#64
mkeeter merged 1 commit intomkeeter:mainfrom
Wulfsta:if_nonzero_else

Conversation

@Wulfsta
Copy link
Copy Markdown
Contributor

@Wulfsta Wulfsta commented Apr 8, 2024

This implements an if_nonzero_else helper function as implied in #58. Let me know if a Heaviside step function and if_nonnegative_else would be useful as well.

let lhs = self.and(condition, a)?;
let n_condition = self.not(condition)?;
let rhs = self.and(n_condition, b)?;
self.add(lhs, rhs)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
self.add(lhs, rhs)
self.or(lhs, rhs)

(otherwise, it won't be fully simplified after tracing evaluation)

Copy link
Copy Markdown
Contributor Author

@Wulfsta Wulfsta Apr 8, 2024

Choose a reason for hiding this comment

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

Yes, you are correct - I was thinking about this backwards, as you can tell from my comment in #58. Honestly I keep getting confused by this construction, the "if zero then zero else" nature of it keeps tripping me up for some reason!

@Wulfsta Wulfsta mentioned this pull request Apr 8, 2024
@mkeeter mkeeter merged commit b285ff9 into mkeeter:main Apr 8, 2024
@mkeeter mkeeter mentioned this pull request Apr 8, 2024
mkeeter added a commit that referenced this pull request Apr 8, 2024
This is a follow-up to #64, making a few tweaks:

- Adds a test for conditional simplification
- Adds a short description to the changelog
- Removes the textual form, because I want that format to be 1:1 with
  opcodes
- Removes the unit test, because it's identical to the doctest
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