-
-
Notifications
You must be signed in to change notification settings - Fork 18
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 all your base exercise #219
Conversation
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
@kahgoh The tests seem to be failing. Could you look into that? |
@ErikSchierboom I've just pushed the fixes. |
Still seems to fail:
|
I'm not sure how I missed adding the example implementation in the commit 🤦🏻♂️. I've just pushed a fix. |
I've just noticed the CI is failing for MacOS. I'm having a look into it. |
} | ||
] | ||
}, | ||
"foregone": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be keeping foregone
.
Multithreading exercises like "parallel-letter-frequency"
and "bank-account"
are not applicable for this track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was remove by configlet when I ran configlet fmt
. I think it makes sense to forgo those exercises, so I'll add them in.
; %r8d - output base | ||
|
||
xor rax, rax | ||
xor rbx, rbx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be saving rbx, and restoring the original value before we return.
https://wiki.osdev.org/System_V_ABI#x86-64
"Functions preserve the registers rbx, rsp, rbp, r12, r13, r14, and r15"
Wikipedia - System V AMD64 ABI
"If the callee wishes to use registers RBX, RSP, RBP, and R12–R15, it must restore their original values before returning control to the caller."
At the start:
push rbx
Before return:
pop rbx
Another option would be to not use rbx, and use r11 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omgosh, this was what was causing the failure in the MacOS CI build! Thanks for pointing that out @keiravillekode!
@ErikSchierboom I've just pushed the fix for the failures. Should be ready for review again. |
I'll let @keiravillekode do the code review |
mov r10d, dword [rdi + (r9 * 4)] | ||
|
||
cmp r9d, esi | ||
je .convert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove lines 38, 39.
They are a repeat of lines 32, 33.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great work.
On the MIPS track, I gave this exercise a difficulty of 8, The assembly tracks currently have exceedingly few hard exercises (difficulty 8/9/10), and it would nice to work towards having 5+ hard exercises on each track, so people can earn the "Difficult" track trophy. (See Java for an example of a track with 5+ hard exercises.)
- Remove unneeded jump. - Tidy up comments. - Update difficulty to match MIPS track.
This is one of the 48in24 exercises