Swap l endian - #1
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR pivots the repository toward a Corewar assembler (robot-factory) by adding parsing/encoding logic and new test tooling, while removing a large set of previously included library/unit-test assets that are no longer used.
Changes:
- Added assembler core: header/body parsing, label tracking, instruction sizing, and
.corencoding (big-endian). - Added new Criterion unit tests and a functional test runner that compares generated
.corfiles to reference outputs. - Trimmed
libmyand removed old/unit-test assets that were unrelated to Robot Factory.
Reviewed changes
Copilot reviewed 82 out of 84 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests_robotfactory.c | New Criterion tests for opcode lookup, instruction sizing, and basic parsing expectations. |
| tests/unit_tests_lib.c | Removed prior lib/cuddle unit test suite. |
| tests/tests | Removed legacy functional test definitions file (unrelated to assembler). |
| tests/testin_hello_file | Removed legacy test fixture file. |
| tests/run_tests.sh | Added functional test runner for the assembler (binary comparisons vs reference .cor). |
| tests/Makefile | Updated unit test build to target assembler sources and changed pass/fail detection. |
| Src/Tools/shifter.c | Added integer byte-swap helper for big-endian header values. |
| Src/Parser/parser.c | Added top-level parsing of .name/.comment plus body parsing orchestration. |
| Src/Parser/parser_body.c | Added body parsing: labels, instruction nodes, parameter parsing, size tracking. |
| Src/Parser/is_header_line.c | Added helper to skip header/comment lines during body parse. |
| Src/Parser/get_opcode.c | Added mnemonic-to-opcode lookup based on op_tab. |
| Src/Parser/get_instr_size.c | Added instruction sizing rules including IDX instructions and coding byte rules. |
| Src/Parser/free_robotfactory.c | Added cleanup for allocated instruction/label structures. |
| Src/op.c | Updated op_tab integration and project header metadata; aligned mnemonic aff. |
| Src/main.c | Implemented CLI entrypoint: parse .s, write .cor, return status codes. |
| Src/Headers/robotfactory.h | Expanded project API/types for parser/encoder and added prototypes. |
| Src/Headers/op.h | Minor formatting cleanup in macros/comments. |
| Src/Header/Parser/parser.c | Modified legacy/alternate parser file content (appears unused by main build). |
| Src/Encoder/encoder.c | Added .cor writer: header + opcode/coding byte + params, label resolution. |
| Makefile | Wired new assembler sources into build and updated help/functional test behavior. |
| Doc/schem.mmd | Updated architecture diagram from previous project to Robot Factory flow. |
| Doc/README.md | Rewrote README to describe Robot Factory assembler usage, structure, and targets. |
| lib/my/Makefile | Reduced libmy build inputs to only needed utilities for assembler path. |
| lib/Headers/my.h | Pruned function prototypes to match reduced libmy surface. |
| lib/Headers/linked_lists.h | Pruned list API to only push_back. |
| lib/my/string/my_word_array_to_str.c | Removed unused string helper implementation. |
| lib/my/string/my_strupcase.c | Removed unused string helper implementation. |
| lib/my/string/my_strstr.c | Removed unused string helper implementation. |
| lib/my/string/my_strncmp.c | Removed unused string helper implementation. |
| lib/my/string/my_strncat.c | Removed unused string helper implementation. |
| lib/my/string/my_strlowcase.c | Removed unused string helper implementation. |
| lib/my/string/my_strconcat.c | Removed unused string helper implementation. |
| lib/my/string/my_strcapitalize.c | Removed unused string helper implementation. |
| lib/my/string/my_str_isupper.c | Removed unused string helper implementation. |
| lib/my/string/my_str_isnegnum.c | Removed unused string helper implementation. |
| lib/my/string/my_str_islower.c | Removed unused string helper implementation. |
| lib/my/string/my_str_isfloat.c | Removed unused string helper implementation. |
| lib/my/string/my_str_isalpha.c | Removed unused string helper implementation. |
| lib/my/string/my_revstr.c | Removed unused string helper implementation. |
| lib/my/string/my_nbr_to_str.c | Removed unused string helper implementation. |
| lib/my/string/my_char_isnum.c | Removed unused string helper implementation. |
| lib/my/print/my_str_isprintable.c | Removed unused print helper implementation. |
| lib/my/print/my_show_word_array.c | Removed unused print helper implementation. |
| lib/my/print/my_putstr.c | Removed unused print helper implementation. |
| lib/my/print/my_putnbr_base.c | Removed unused print helper implementation. |
| lib/my/print/my_putchar.c | Removed unused print helper implementation. |
| lib/my/print/my_put_nbr.c | Removed unused print helper implementation. |
| lib/my/print/my_put_float.c | Removed unused print helper implementation. |
| lib/my/memory/free_all.c | Removed unused variadic free helpers. |
| lib/my/math/my_swap.c | Removed unused math helper implementation. |
| lib/my/math/my_sort_int_array.c | Removed unused math helper implementation. |
| lib/my/math/my_isneg.c | Removed unused math helper implementation. |
| lib/my/math/my_is_prime.c | Removed unused math helper implementation. |
| lib/my/math/my_find_prime_sup.c | Removed unused math helper implementation. |
| lib/my/math/my_compute_square_root.c | Removed unused math helper implementation. |
| lib/my/math/my_compute_power_rec.c | Removed unused math helper implementation. |
| lib/my/math/minimum_int_from_list.c | Removed unused math helper implementation. |
| lib/my/math/len_nb.c | Removed unused math helper implementation. |
| lib/my/list/search_in_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/push_front.c | Removed unused linked-list helper implementation. |
| lib/my/list/pop_front.c | Removed unused linked-list helper implementation. |
| lib/my/list/pop_back.c | Removed unused linked-list helper implementation. |
| lib/my/list/pop_at.c | Removed unused linked-list helper implementation. |
| lib/my/list/my_wordarray_to_linked_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/my_linked_list_to_word_array.c | Removed unused linked-list helper implementation. |
| lib/my/list/merge_sort_split_lists.c | Removed unused linked-list helper implementation. |
| lib/my/list/merge_sort_merge_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/list_len.c | Removed unused linked-list helper implementation. |
| lib/my/list/invert_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/insert_node.c | Removed unused linked-list helper implementation. |
| lib/my/list/init_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/get_to_middle.c | Removed unused linked-list helper implementation. |
| lib/my/list/get_front.c | Removed unused linked-list helper implementation. |
| lib/my/list/get_back.c | Removed unused linked-list helper implementation. |
| lib/my/list/get_at.c | Removed unused linked-list helper implementation. |
| lib/my/list/free_list.c | Removed unused linked-list helper implementation. |
| lib/my/list/attach_list_to_back.c | Removed unused linked-list helper implementation. |
| lib/my/io/get_file_size_stat.c | Removed unused IO helper implementation. |
| lib/my/array/sort_word_array.c | Removed unused array helper implementation. |
| lib/my/array/my_char_array_dup.c | Removed unused array helper implementation. |
| lib/my/array/my_array_len.c | Removed unused array helper implementation. |
| lib/my/array/cmp_alpha.c | Removed unused array helper implementation. |
Comments suppressed due to low confidence (1)
tests/Makefile:57
tests_runignores the unit test binary exit code (|| true) and instead greps for specific Criterion output strings ("Failing: 0" / "Crashing: 0"). This is brittle across Criterion versions/configs and can produce false positives/negatives. Prefer using Criterion’s actual exit status (remove|| trueand fail on non-zero), optionally keeping the log for debugging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #include <stdio.h> | ||
| #include "op.h" | ||
| #include "../../include/my.h" |
There was a problem hiding this comment.
robotfactory.h includes ../../include/my.h, but the repository has no include/ directory. This will make the project fail to compile. Update the include to point at the actual lib header (e.g. lib/Headers/my.h) or include it via the configured -I paths (e.g. #include "my.h").
Suggested change
| #include "../../include/my.h" | |
| #include "my.h" |
Comment on lines
+8
to
+13
| #include <criterion/criterion.h> | ||
| #include <criterion/redirect.h> | ||
| #include <stdio.h> | ||
| #include <unistd.h> | ||
| #include "../Headers/robotfactory.h" | ||
| static void write_file_or_fail(const char *path, const char *content) |
There was a problem hiding this comment.
This test includes "../Headers/robotfactory.h", but there is no top-level Headers/ directory (the header lives under Src/Headers/). The include should be adjusted (e.g. include "robotfactory.h" and rely on -I ../Src/Headers, or use the correct relative path) so the unit tests compile.
Comment on lines
+8
to
+11
| int shift_int(int n) | ||
| { | ||
| return (n & 0xFF000000) >> 24 | (n & 0x00FF0000) >> 8 | ||
| | (n & 0x0000FF00) << 8 | (n & 0x000000FF) << 24; |
There was a problem hiding this comment.
shift_int performs bit shifts on a signed int. For values with the high bit set, right shifts can sign-extend and produce incorrect byte swaps, and left shifts can invoke undefined behavior. Use an unsigned type (uint32_t) and cast before shifting/masking to make the endianness conversion well-defined.
Suggested change
| int shift_int(int n) | |
| { | |
| return (n & 0xFF000000) >> 24 | (n & 0x00FF0000) >> 8 | |
| | (n & 0x0000FF00) << 8 | (n & 0x000000FF) << 24; | |
| #include <stdint.h> | |
| int shift_int(int n) | |
| { | |
| uint32_t u = (uint32_t)n; | |
| return (int)(((u & 0xFF000000U) >> 24) | ((u & 0x00FF0000U) >> 8) | |
| | ((u & 0x0000FF00U) << 8) | ((u & 0x000000FFU) << 24)); |
| if (!out || len < 3) | ||
| return free(out), NULL; | ||
| if (input[len - 2] != '.' || input[len - 1] != 's') | ||
| return NULL; |
There was a problem hiding this comment.
get_outfile() allocates out but returns NULL on extension mismatch without freeing it, leaking memory. Free out on all early-return paths (or validate the extension before allocating).
Suggested change
| return NULL; | |
| return free(out), NULL; |
Comment on lines
+10
to
+29
| static void parse_name(char *line, robotfactory_t *rf) | ||
| { | ||
| char **split = my_str_to_word_array(line, "\""); | ||
|
|
||
| if (!split) | ||
| return; | ||
| if (split[1]) | ||
| my_strcpy(rf->header.prog_name, split[1]); | ||
| free_array(split); | ||
| } | ||
|
|
||
| static void parse_comment(char *line, robotfactory_t *rf) | ||
| { | ||
| char **split = my_str_to_word_array(line, "\""); | ||
|
|
||
| if (!split) | ||
| return; | ||
| if (split[1]) | ||
| my_strcpy(rf->header.comment, split[1]); | ||
| free_array(split); |
There was a problem hiding this comment.
parse_name/parse_comment copy user-provided strings into fixed-size header_t buffers using my_strcpy before checking length. If the quoted string exceeds the Corewar limits, this can overflow prog_name/comment and cause memory corruption. Validate split[1] length (or use a bounded copy) before writing into the header buffers.
Comment on lines
+71
to
+83
| if (!raw) { | ||
| write_error("Cannot read file"); | ||
| return FAILURE; | ||
| } | ||
| lines = my_str_to_word_array(raw, "\n"); | ||
| file_cntnt = my_str_to_array_of_word_array(raw, "\n", " \t"); | ||
| result = fill_header(lines, file_cntnt, rf); | ||
| if (result == SUCCESS) | ||
| result = get_body(lines, file_cntnt, rf); | ||
| rf->header.magic = shift_int(COREWAR_EXEC_MAGIC); | ||
| rf->header.prog_size = shift_int(rf->prog_size); | ||
| free_array(lines); | ||
| free_array_of_word_arrays(file_cntnt); |
There was a problem hiding this comment.
parse() doesn't check whether my_str_to_word_array or my_str_to_array_of_word_array returned NULL. On allocation/tokenization failure this will crash in fill_header() / free_array(). Add NULL checks and ensure raw is freed on failure paths.
Comment on lines
+65
to
+75
| static int resolve_param_value(param_t *param, int offset, node_t *labels) | ||
| { | ||
| int pos; | ||
|
|
||
| if (!param->is_label) | ||
| return param->value; | ||
| pos = get_label_pos(labels, param->label); | ||
| if (pos == -1) | ||
| return 2147483647; | ||
| return pos - offset; | ||
| } |
There was a problem hiding this comment.
Label resolution failure is signaled via the magic value 2147483647 (and compared later) rather than an explicit error/return code. This sentinel can collide with a valid immediate value and makes the control flow harder to follow. Prefer returning a status (e.g., boolean + out-param) and emitting a helpful error message when a label is undefined; also consider using the existing MAX_INT macro if you keep a sentinel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.