exercism / x86-64-assembly

Exercism exercises in x86-64 Assembly.
https://exercism.org/tracks/x86-64-assembly
MIT License
22 stars 18 forks source link

Add exercise protein-translation #204

Closed D3usXMachina closed 2 weeks ago

D3usXMachina commented 6 months ago

As discussed here.

There's a little helper function in protein_translation_test.c that reduces the boilerplate stuff a little bit. But this means that the line number shown in case of a failure will not be very useful. Is that an issue? It still shows the name of the test, so I thought it shouldn't be much of a problem.

github-actions[bot] commented 6 months ago

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.

danilopiazza commented 6 months ago

I you want to, you can use a Python script to generate the test cases from canonical data.

Something like this:

FUNC_PROTO = """\
#include "vendor/unity.h"

extern const char **proteins(const char *rna);

void perform_test(const char *rna_strand, const char **expected, int expected_size) {
    const char **names = proteins(rna_strand);
    int size;
    for (size = 0; names[size]; size++) {}

    TEST_ASSERT_EQUAL_INT(expected_size, size);
    if (expected_size > 0) {
        TEST_ASSERT_EQUAL_STRING_ARRAY(expected, names, size);
    }
}
"""

def gen_func_body(_, inp, expected):
    str_list = []
    strand = inp["strand"]
    if "error" in expected or not expected:
        str_list.append(f'perform_test("{strand}", NULL, 0);\n')
    else:
        proteins_array = str(expected).replace("'", '"').replace("[", "{").replace("]", "}")
        str_list.append(f'perform_test("{strand}", (const char *[]){proteins_array}, {len(expected)});\n')
    return "".join(str_list)

I believe using a test generator is encouraged, but not mandatory.

D3usXMachina commented 6 months ago

Thanks for the suggestion. I took the opportunity to refresh my python skills a little bit and added another commit:

https://github.com/D3usXMachina/exercism-x86-64-assembly/commit/d30793fd4ed0ffae1e653f9ca27ad21537dd7a7e

I guess this PR is going to update once it is reopened?

ErikSchierboom commented 6 months ago

Could you fix the CI failures?

D3usXMachina commented 6 months ago

Should be fixed now

ErikSchierboom commented 6 months ago

@D3usXMachina it looks like this fails on macos.

D3usXMachina commented 6 months ago

No need to run the conflglet yet. I haven't yet checked what the issue on macos might be.

D3usXMachina commented 6 months ago

So I tried to look into the issue on MacOS, but unfortunately with limited success.

I ran it on a VM with running MacOS 22.6.0, Apple clang 15.0.0, and nasm 2.14.02.

I had to remove the --fatal-warnings flag and add void arguments to the tests for linked-list, but then it ran fine besides (ignoring a linker warning).

However, it still fails the workflow with a segmentation fault when running the protein-translation tests. Also, since linked-list passed the workflow, I assume the lack of void arguments wasn't actually an issue.

The output from the verify-exercises runs after each alteration are attached.

1_as_is.txt 2_no_fatal_warnings.txt 3_add_void_arguments.txt

Does anybody have any ideas or suggestions?

bergjohan commented 6 months ago

So I tried to look into the issue on MacOS, but unfortunately with limited success.

I ran it on a VM with running MacOS 22.6.0, Apple clang 15.0.0, and nasm 2.14.02.

I had to remove the --fatal-warnings flag and add void arguments to the tests for linked-list, but then it ran fine besides (ignoring a linker warning).

However, it still fails the workflow with a segmentation fault when running the protein-translation tests. Also, since linked-list passed the workflow, I assume the lack of void arguments wasn't actually an issue.

The output from the verify-exercises runs after each alteration are attached.

1_as_is.txt 2_no_fatal_warnings.txt 3_add_void_arguments.txt

Does anybody have any ideas or suggestions?

A quick google suggests adding the linker flag -Wl,-ld_classic. Something about the new linker in Xcode 15 breaking stuff.

D3usXMachina commented 6 months ago

So I tried to look into the issue on MacOS, but unfortunately with limited success. I ran it on a VM with running MacOS 22.6.0, Apple clang 15.0.0, and nasm 2.14.02. I had to remove the --fatal-warnings flag and add void arguments to the tests for linked-list, but then it ran fine besides (ignoring a linker warning). However, it still fails the workflow with a segmentation fault when running the protein-translation tests. Also, since linked-list passed the workflow, I assume the lack of void arguments wasn't actually an issue. The output from the verify-exercises runs after each alteration are attached. 1_as_is.txt 2_no_fatal_warnings.txt 3_add_void_arguments.txt Does anybody have any ideas or suggestions?

A quick google suggests adding the linker flag -Wl,-ld_classic. Something about the new linker in Xcode 15 breaking stuff.

That makes it fail at the linking stage:

 ld: library not found for -ld_classic
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Also, this seems to be only affecting protein-translation, so I guess I must be doing something either in the tests or in the example solution that causes the seg fault.

keiravillekode commented 4 weeks ago

Are you still planning to work on this, or should I upload a fork of this PR?

D3usXMachina commented 3 weeks ago

Are you still planning to work on this, or should I upload a fork of this PR?

I was planning to, but it hasn't been a high priority. I'll try to find time this week. Feel free to take over if I haven't managed to do it by the end of the week.

keiravillekode commented 3 weeks ago

Here are a few thoughts. I apologise for not thinking about these earlier.

A. It might be better for the client to pass in the destination buffer:

extern void proteins(const char **dest, const char *rna);

This has at least three benefits:

  1. The function becomes useful in multi-threaded contexts. Suppose there are multiple client threads. With the current interface, a client thread would need to claim a lock, call the function, process the output, and then release the lock so another client thread could proceed. If we eliminate the statically allocated output: resq 10 then none of that is needed. Parallelism is now only limited by hardware.

  2. We don't know how long our clients' strands might be. A person's DNA can be 700 MB, I don't know much about RNA. Responsibility for providing sufficient buffer space can be transferred to the client. The test cases can stack-allocate a length 10 array.

  3. Students won't be asking (themselves) if they need to call _malloc to allocate memory. This could also be addressed by adding a .docs/instructions.append.md file, but providing a dest argument is a way to make things clear.

B. We can git rm exercises/practice/protein-translation/.meta/create_tests.py

The generators/exercises/protein_translation.py is much appreciated.

C. Consider adding a .docs/instructions.append.md file (example) that explains students should write a null pointer at the end of the output.

Note the first line of this file is typically

# Instructions append

and doesn't get shown to students.