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 all your base exercise #219

Merged
merged 8 commits into from
Jun 13, 2024
Merged

Add all your base exercise #219

merged 8 commits into from
Jun 13, 2024

Conversation

kahgoh
Copy link
Member

@kahgoh kahgoh commented Jun 2, 2024

This is one of the 48in24 exercises

Copy link

github-actions bot commented Jun 2, 2024

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.

@github-actions github-actions bot closed this Jun 2, 2024
@ErikSchierboom ErikSchierboom reopened this Jun 4, 2024
@ErikSchierboom
Copy link
Member

@kahgoh The tests seem to be failing. Could you look into that?

@kahgoh
Copy link
Member Author

kahgoh commented Jun 5, 2024

@ErikSchierboom I've just pushed the fixes.

@ErikSchierboom
Copy link
Member

Still seems to fail:

all_your_base_test.c:24:test_single_bit_one_to_decimal:FAIL: Element 0 Expected 1 Was 0
all_your_base_test.c:33:test_binary_to_single_decimal:FAIL: Expected 1 Was 5
all_your_base_test.c:43:test_single_decimal_to_binary:FAIL: Expected 3 Was 1
all_your_base_test.c:53:test_binary_to_multiple_decimal:FAIL: Expected 2 Was 4
all_your_base_test.c:63:test_decimal_to_binary:FAIL: Expected 6 Was 1
all_your_base_test.c:74:test_trinary_to_hexadecimal:FAIL: Element 0 Expected 2 Was 0
all_your_base_test.c:83:test_hexadecimal_to_trinary:FAIL: Expected 4 Was 2
all_your_base_test.c:93:test_15bit_integer:FAIL: Expected 3 Was 45
all_your_base_test.c:103:test_empty_list:FAIL: Expected 1 Was -1240367940
all_your_base_test.c:113:test_single_zero:FAIL: Expected 1 Was -1240367940
all_your_base_test.c:123:test_multiple_zeros:FAIL: Expected 1 Was -1240367940
all_your_base_test.c:133:test_leading_zeros:FAIL: Expected 2 Was 4
all_your_base_test.c:142:test_input_base_is_one:FAIL: Expected -1 Was -1240367524
all_your_base_test.c:150:test_input_base_is_zero:FAIL: Expected -1 Was 0
all_your_base_test.c:158:test_input_base_is_negative:FAIL: Expected -1 Was 1
all_your_base_test.c:166:test_negative_digit:FAIL: Expected -2 Was 1
all_your_base_test.c:174:test_invalid_positive_digit:FAIL: Expected -2 Was 1
all_your_base_test.c:182:test_output_base_is_one:FAIL: Expected -1 Was 1
all_your_base_test.c:190:test_output_base_is_zero:FAIL: Expected -1 Was 7
all_your_base_test.c:198:test_output_base_is_negative:FAIL: Expected -1 Was 1
make: *** [Makefile:29: all] Error [21](https://github.com/exercism/x86-64-assembly/actions/runs/9384592753/job/25843210083?pr=219#step:5:22)
all_your_base_test.c:206:test_both_bases_are_negative:FAIL: Expected -1 Was 1

@kahgoh
Copy link
Member Author

kahgoh commented Jun 6, 2024

I'm not sure how I missed adding the example implementation in the commit 🤦🏻‍♂️. I've just pushed a fix.

@kahgoh
Copy link
Member Author

kahgoh commented Jun 7, 2024

I've just noticed the CI is failing for MacOS. I'm having a look into it.

}
]
},
"foregone": [
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

@keiravillekode keiravillekode Jun 10, 2024

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.

Copy link
Member Author

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!

@kahgoh
Copy link
Member Author

kahgoh commented Jun 10, 2024

@ErikSchierboom I've just pushed the fix for the failures. Should be ready for review again.

@ErikSchierboom
Copy link
Member

I'll let @keiravillekode do the code review

mov r10d, dword [rdi + (r9 * 4)]

cmp r9d, esi
je .convert
Copy link
Contributor

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.

Copy link
Contributor

@keiravillekode keiravillekode Jun 12, 2024

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.
@ErikSchierboom ErikSchierboom merged commit d0e2c0a into exercism:main Jun 13, 2024
4 checks passed
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