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

Add bitwise concept #722

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

stewartmurrie
Copy link

This addresses issue #698 and adds a concept and exercise for bitwise operations. It's based on the Java track's bit-manipulation concept and corresponding Secrets exercise.

I've checked that the exemplar passes the test suite, and the configlet linter is happy (with my stuff at least). Let me know if there's something else I should be doing before this can be merged.

@jiegillet jiegillet self-requested a review November 6, 2024 00:08
Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Thank you so much @stewartmurrie this is great!
I know how much work goes into adding concepts.

First, purely for your reference, here is my solution to the exercise by only looking at the instructions:

module Secrets exposing (..)

import Bitwise

shiftBack =
    Bitwise.shiftRightZfBy 

setBits =
    Bitwise.or

flipBits =
    Bitwise.xor

clearBits mask value =
    Bitwise.and (Bitwise.complement mask) value

It's a little bit on the simple side, but clearBits saves it by not being just an alias 😄 .

I left some small suggestions, and tons of tiny ones (sorry about that, I tend to be super nitpicky for these kinds of PRs), but overall, this is in great shape.

config.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
concepts/bitwise-operations/links.json Outdated Show resolved Hide resolved
concepts/bitwise-operations/introduction.md Outdated Show resolved Hide resolved
exercises/concept/secrets/.meta/Exemplar.elm Outdated Show resolved Hide resolved
exercises/concept/secrets/.meta/Exemplar.elm Outdated Show resolved Hide resolved
exercises/concept/secrets/src/Secrets.elm Outdated Show resolved Hide resolved
exercises/concept/secrets/tests/Tests.elm Outdated Show resolved Hide resolved
exercises/concept/secrets/tests/Tests.elm Outdated Show resolved Hide resolved
@ceddlyburge
Copy link
Contributor

Hi everyone, it looks like there are still some comments to address, please let me know if you want anything from me.
Thanks, Cedd

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Fantastic work @stewartmurrie, I'm very happy with this :)

I left a few more comments, but I'm approving already.
@ceddlyburge did you want to take a look also?

exercises/concept/secrets/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/secrets/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/secrets/.docs/instructions.md Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
# Instructions
Copy link
Contributor

Choose a reason for hiding this comment

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

It's perfect :)
Nice call replacing "your birth year" with the year you met.

concepts/bitwise-operations/introduction.md Outdated Show resolved Hide resolved
concepts/bitwise-operations/introduction.md Outdated Show resolved Hide resolved
concepts/bitwise-operations/introduction.md Outdated Show resolved Hide resolved
So, negative numbers will stay negative:

```elm
Bitwise.shiftRightBy 3 -21 --> -6
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not what I'm getting from elm repl

> import Bitwise
> import BitwisBitwise.shiftRightBy 3 -21
-3 : Int

also -21 should be 111...101011 according to my calculator app.

If you want to shift right and fill in with zeros, use `shiftRightZfBy`:

```elm
Bitwise.shiftRightZfBy 3 -21 --> 1073741818
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also not what I'm getting, I get

> Bitwise.shiftRightZfBy 3 -21
536870909 : Int

@ceddlyburge
Copy link
Contributor

@ceddlyburge did you want to take a look also?
I'm taking a quick look now

-- and = 01000 = 8


### or
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a level 4 heading

-- or = 10111 = 23
````

### Exclusive-or (xor)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a level 4 heading


````elm
Bitwise.xor 21 4 --> 17
-- 21 = 10101
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to use 20 and 5 here, so that you can see that it works both ways round. ( 0 - 1 and 1 - 0 )

-- 13 = 01101
-- 8 = 01000
-- and = 01000 = 8

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra blank line here I think

@ceddlyburge
Copy link
Contributor

I left some very minor comments, I'll approve it after that. I don't think they will be difficult to implement or controversial.

@jiegillet
Copy link
Contributor

@stewartmurrie do you still have time to finish this up? Or do you want some help? We are not in any particular rush, but we are also happy to help.

I actually realized one more thing this PR is missing, the exercises/concept/secrets/.meta/design.md file. That file is pretty much the raw content of the issue description, minus the comments section, plus the requirements for the analyzer (in this case, we could check that the last function reuses all of the previously defined ones, although if we do that, we should request it explicitly in the instructions).

@stewartmurrie
Copy link
Author

Yup yup, I'll finish this one out; thanks for checking in :)

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