Skip to content

Add partial applicative typeclass functions#2545

Merged
martijnbastiaan merged 2 commits intomasterfrom
add-partial-applicative-typeclass-functions
Jun 5, 2025
Merged

Add partial applicative typeclass functions#2545
martijnbastiaan merged 2 commits intomasterfrom
add-partial-applicative-typeclass-functions

Conversation

@lmbollen
Copy link
Copy Markdown
Member

@lmbollen lmbollen commented Jul 14, 2023

We currently have functions like .==. that can operate on Signals. However, I find myself often writing .foo. pure bar when comparing signals to constants.

With these changes(inspired by @gergoerdi's retrocomputing lib) we can write .foo bar instead.

Still TODO:

  • Determine if there are more operators we'd like to add signal functions for.
  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@lmbollen lmbollen force-pushed the add-partial-applicative-typeclass-functions branch from cd095db to 9158361 Compare July 18, 2023 07:39
Comment thread clash-prelude/src/Clash/Signal/Internal.hs Outdated
@martijnbastiaan
Copy link
Copy Markdown
Member

martijnbastiaan commented Feb 16, 2025

This makes sense to me. Can we also get (.&&), (&&.), (.||), (||.)? That seems to cover all the operators we export. Are you interested in moving this out of draft @lmbollen?

Edit: What does bother me a little bit is that we use both . for signal lifting (like in this PR) and for BitVector operations (like +>>.).

@DigitalBrains1
Copy link
Copy Markdown
Member

DigitalBrains1 commented Feb 16, 2025

The commit details for the first commit say:

We already have the ' .foo.' pattern for these typeclasses.
However, you often have to use '.foo . pure bar' if 'bar' is a constant.
After this change, you can use '.foo bar' instead.

Note the stray space between .foo and . pure bar.

And a bit nitpicky, the commit summary goes over the 50-char limit of the 50/72 rule. I didn't count the characters, I noticed the GitHub website UI cut off the summary: 24d7979

[edit]
I also suspect you were going for Markdown formatting when you included single quotes in the commit message, but in that case you probably meant backticks. All these formatting languages... :-S
[/edit]

[edit 2]
According to this blogpost the ellipsis only shows when the summary goes over 72. I feel more than 72 truly is too long.
[/edit 2]

@lmbollen
Copy link
Copy Markdown
Member Author

Are you interested in moving this out of draft @lmbollen?
Yeah I'd really like to have these operators. I hope to be able to update the PR soon.

Edit: What does bother me a little bit is that we use both . for signal lifting (like in this PR) and for BitVector operations (like +>>.).

Yes, though I feel the . is more useful as a lifting indicator since it can be applied a lot more..
For BitVector we only have .<<+ and +>>. which are derived from their Vector counterparts <<+ and +>>

@martijnbastiaan
Copy link
Copy Markdown
Member

Yeah, I guess my comment wasn't really directed at this PR. I was merely pointing out the fact that +>>. is ambiguous: is it +>> lifted over Signal, or is it the BitVector version? We should probably think of an alternative notation for the latter..

@DigitalBrains1
Copy link
Copy Markdown
Member

I agree in principle, but the whole problem with these comic book swear words is that even if you think of a rigorous structure and apply it, it will always feel random without a full explanation of why . means A and ^ means B because the common real-world meaning of the glyph is usually not in any way connected to how you use it. The meaning of the dot punctuation, in most Roman languages, is that it ends a sentence. Three of them form an ellipsis... and so on. We just lift it to mean lifting. Heh.

@DigitalBrains1
Copy link
Copy Markdown
Member

We could just call them bitShiftInL and bitShiftInR and use them as

x `bitShiftInR` y

@lmbollen lmbollen force-pushed the add-partial-applicative-typeclass-functions branch 3 times, most recently from d98ad49 to 433b2a7 Compare March 19, 2025 08:07
@lmbollen lmbollen marked this pull request as ready for review March 19, 2025 08:08
@lmbollen lmbollen force-pushed the add-partial-applicative-typeclass-functions branch from 433b2a7 to 3a2398c Compare March 20, 2025 08:14
Copy link
Copy Markdown
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

LGTM modulo missing exports and a CI breakage :)

Comment thread clash-prelude/src/Clash/Signal.hs
Comment thread changelog/2025-03-19T08_52_01+01_00_new_partial_signal_operators Outdated
@lmbollen lmbollen force-pushed the add-partial-applicative-typeclass-functions branch from 3a2398c to c0dc122 Compare June 5, 2025 07:46
lmbollen added 2 commits June 5, 2025 10:04
We already have the `.foo.` pattern for these typeclasses.
After this change, you can use `.foo bar` instead when `bar` is a constant.
@lmbollen lmbollen force-pushed the add-partial-applicative-typeclass-functions branch from c0dc122 to c4c50a9 Compare June 5, 2025 08:05
@martijnbastiaan martijnbastiaan enabled auto-merge June 5, 2025 08:13
@martijnbastiaan martijnbastiaan merged commit a3176ef into master Jun 5, 2025
14 checks passed
@martijnbastiaan martijnbastiaan deleted the add-partial-applicative-typeclass-functions branch June 5, 2025 09:07
@martijnbastiaan martijnbastiaan mentioned this pull request Oct 5, 2025
2 tasks
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.

4 participants