Fix all compilation warnings and errors. - #72
Merged
Conversation
Owner
|
All done, but the coding style didn't pass 😢 |
Still 604 to fix
Port the codebase from SFML to the CSFML 3.x binding to restore a working build on the current toolchain: - includes moved from <SFML/*> to <CSFML/*> - SFML 3 breaking API changes applied: sfIntRect/sfFloatRect now expose .position/.size vectors instead of left/top/width/height, and pixel helpers (sfImage_getPixel, ...) take sfVector2u/sfVector2f arguments - Makefile updated with the CSFML Homebrew include/lib paths Temporary startup debug logging is included to trace initialisation on macOS; it is removed in the stabilisation pass.
Add the my_rpg binary, coverage outputs and the test runner to the ignore list, plus a set of local-only working documents (specs, scratch notes) that must never be committed.
The TCP client path is the main source of instability and carries an out-of-bounds write: receive_clients_infos() indexes clients[] with an unbounded value read straight from the socket. For the solo-reliability phase the connection is simply never opened (main no longer calls connect_to_server), so game->network stays NULL and every networked path short-circuits through its existing NULL guards. The network layer is left in place to be hardened and re-enabled later. main.c is brought to Banana compliance along the way: debug logging removed, lifecycle prototypes (is_error/start_game/do_free) moved to game.h instead of being forward-declared in the source file (C-H1), and the local declaration moved to the top of the function (C-L5).
Stabilise the boot sequence and bring the entry-point files to Banana compliance: - start_game: check every malloc (keys/clock/perso/params) and bail out with EPITECH_ERROR instead of dereferencing NULL on OOM; split the oversized init into init_keys/init_game/init_params to respect the 20-line rule; drop the leftover debug logging and the dead network disconnect (networking is disabled for solo play). - error: make is_error portable — no DISPLAY requirement on macOS, real env DISPLAY check on Linux — and remove the in-body comments and the unused string-helper forward declarations. Introduce per-subsystem prototype headers (my_str, create, display, events, raycasting_functions) so cross-file functions are declared in headers instead of being forward-declared in source files (C-H1). This is the first step of breaking the implicit god-header coupling.
Several core helpers were either incorrect or unsafe; they are rewritten and verified against edge cases (0, negatives, INT_MIN, INT_MAX): - my_itoa: fixed-size 10-byte buffer overflowed on values needing more than 9 digits (e.g. INT_MIN = "-2147483648", 12 chars) and ignored negatives entirely. Now sizes the buffer from the value and handles the full int range via a long accumulator. - my_getnbr: previously summed every digit found anywhere in the string and toggled the sign on each '-'. Now skips leading blanks, applies the sign, and stops at the first non-digit. - my_strcmp / my_strncmp: returned 1/0 from an O(n^2) double loop and had an off-by-one on n. Reimplemented to the standard signed-difference contract; my_strncmp now compares exactly n characters. - my_put_nbr: mishandled 0 and the sign. Rewritten with a long accumulator so INT_MIN prints correctly. - my_strcat / my_strndup: check the allocation before writing. - my_float: replaced the convoluted rounding path with a straightforward integer/fraction split. All helpers now declare their prototypes through my_str.h instead of ad-hoc forward declarations (C-H1) and pass Banana with zero findings.
parse_map walked off the end of the buffer when the dimension separator '/' or the newline was missing, under-allocated and never initialised the cell array (leaving later reads on uninitialised memory), and none of open/malloc were checked. read_file leaked the file descriptor on a failed read, and get_map leaked the whole file buffer on every call. Rewritten with bounded scanning (parse_dimensions validates the '/' and newline are present and that width/height are strictly positive), calloc so missing cells default to 0, and full error handling on every syscall and allocation. Verified against valid, separator-less, truncated and missing-file inputs — no crash, no out-of-bounds. Internal helpers are now static and get_map is declared in raycasting_functions.h (C-H1); zero Banana findings.
The raycasting engine indexed the tile array from raw detection coordinates with no bounds, and search_wall marched the ray in a while (1) loop with no exit: as soon as a ray left the map (an open edge, or the player slipping out of bounds) it read arbitrary memory and could spin forever. Introduce a single bounded accessor, raycast_cell(), that treats any out-of-range or zero-sized cell as a wall. Every tile lookup in get_wall, define_wall and the collision test now goes through it, and the ray march is capped at MAX_RAY_DEPTH. Negative player coordinates can no longer produce a negative index. The accessor also removes the duplicated index arithmetic, which let wall_orientation be rewritten from one unreadable multi-line condition into four named neighbour reads. raycast_cell takes the map by pointer (C-F7), helpers are static (C-H1), and the four files are Banana-clean. Verified: raycast_cell returns "wall" for negative, past-edge and far out-of-range indices without crashing.
Bring the remaining raycasting files to Banana compliance without changing the rendering maths: - calculate_entity: declarations moved to the top of each function, the entity projection split out of calculate_entity_form to respect the 20-line limit, and the helpers made static (C-H1). - raycasting: drop the in-source forward declarations in favour of raycasting_functions.h, hoist loop indices to the function top, and extract the entity-visibility test out of the deeply nested condition in display_scene. Guard display_wall against a negative texture index (sprite id 0 previously indexed textures[-1]). - sort_walls: keep the existing exchange-sort behaviour but hoist the swap temporary and loop counters to the top; swap_rays is now static. All seven raycasting translation units are Banana-clean.
Three runtime hazards in the 2D collision and mob code: - collisions: the pixel lookup cast possibly-negative world coordinates straight to unsigned and fed them to sfImage_getPixel with no bounds check, so an entity near an edge read outside the collision image. The lookup now goes through is_solid_pixel(), which clamps against the image size and treats anything out of range as solid. - set_vector_speed: scaled the vector up in a while loop that never terminated when a component was 0 (a still mob, or an axis-aligned offset). Replaced with a direct normalise-to-length that returns the zero vector untouched. - manage_mobs: the player-distance used the mob speed as the power exponent instead of squaring, so aggro range was wrong for any speed other than 2. Fixed to a real Euclidean distance. is_colliding, move_ennemi and set_vector_speed now take their vectors by pointer (C-F7) and are declared in collisions.h / mobs_functions.h (C-H1). display/perso.c is updated to the new collision signature and cleaned up (statements split, declarations hoisted, helpers extracted). Verified: set_vector_speed no longer hangs on a zero vector and normalises correctly.
The save subsystem had a descriptor/buffer leak and read past the parsed data on any short or corrupted file: - get_text leaked the getline buffer on every load and kept a redundant per-line copy path; it now strips the newline in place and frees the buffer once. - load_save never checked fopen and indexed text[0..30] blindly, so a truncated or hand-edited save read out of bounds. It now bails on a failed open and only parses when at least SAVE_FIELDS (31) lines are present, freeing the table otherwise. - save() checked for a free slot but opened O_RDWR without verifying the descriptor; it now opens write-only and handles open failure. - add_str / tab_null / my_strdup check their allocations; my_strdup is now file-local (it was only used here). write_params/keys/perso were factored through a write_int helper to stay under the line limit, and every save function is declared in save_functions.h instead of being forward-declared in source (C-H1). Verified: loading a missing or truncated file no longer crashes or reads out of bounds.
Remove in-source forward declarations in favour of create.h / start.h (C-H1), drop the blank lines inside statement sections (C-L6), hoist loop counters to the function top (C-L5), and split create_scene so each function stays under the 20-line limit. create_window also drops its startup debug logging. Behaviour unchanged; go_back is now declared in start.h. Files: go_back, map, end, inventory, overlay, window, index.
- dialog: close the fd and free the buffer that put_dialog leaked on every load, check open/malloc/read, and split statements onto their own lines. - save: free the my_itoa/my_strcat intermediates that add_img leaked, and split the menu title into its own builder. - start_menu / npc: pass the size/pos/rect structs by pointer (C-F7); npc layout data moves to file-scope const tables so create_each_npc stays under the line limit. - forward declarations replaced by create.h / my_str.h includes (C-H1), loop counters hoisted, blank lines inside statement bodies removed. Files: dialog, save, start_menu, npc, clients, option.
The five settings-panel builders shared the same issues: create_button forward-declared in source, button label arrays and loop counters declared mid-function, and blank lines inside statement bodies. Replaced forward declarations with create.h, hoisted the data and counters to the function top, and extracted per-button setup / music container/bar helpers so each function stays under the line limit. Behaviour unchanged. Files: navbar, fps, music, keyboard, options/window.
Last create/ batch, completing the subsystem at zero findings: - raycasting: guard against a NULL get_map result (a missing map file used to be dereferenced and crash), free the map wrappers, and fix the maps array element size (was sizeof(map_t)). Helpers made static. - raycasting_entities / mobs: pass position/rect structs by pointer (C-F7); mob spawn coordinates move to file-scope const tables and are spawned through a small batch helper, replacing ~30 by-value calls. - menu / character / quest: the two create_text helpers are now static, take the position by pointer and stay within the 4-parameter limit (character reads the shared menu font); a leaked per-title malloc is dropped. All create/ translation units are Banana-clean.
Dispatcher and small renderers: drop in-source forward declarations for display.h (C-H1), flatten the map-visibility condition behind a helper, hoist loop-body declarations to the function top (C-L5), turn the go_back mouse-click ternary into an if (C-C2), make fade_out static and split its compound statement, and fix continuation indentation. fade_out and change_to_game keep their behaviour. Files: index, window, inventory_bar, go_back, change_to_game, end.
Pass the mouse-position struct by pointer in the save and start-menu hover tests (C-F7), turn the start-menu click ternary into an if (C-C2), and route manage_mobs through mobs_functions.h. Loop-body declarations hoisted, blank lines inside statement bodies removed, and oversized draw loops split into per-item/per-slot/per-button helpers to respect the line limit. Files: save, mobs, start_menu, overlay, menu/inventory, menu/character.
- dialog: remove two unused file-scope globals (C-G4), split the event loop into a helper and replace the close/keypress ternaries with ifs. - npc: pass the computed position by pointer (C-F7) and flatten the nested interaction logic into try_interact. - clients: declare the network entry points through network_functions.h (C-H1) instead of forward-declaring them in source. - options: pass the mouse position by pointer (C-F7), turn the activate ternary into an if, and split the panel/background drawing into helpers while preserving the original draw order. diplay_text and relase_button are now declared in display.h.
Last display/ batch, bringing the subsystem to zero findings: - pass every mouse-position struct by pointer (C-F7) and replace the click/hover ternaries with ifs (C-C2) across the menu sidebar, music, keyboard, fps and window-settings panels. - split the oversized hover/draw loops into per-button helpers to stay under the line limit, hoist loop counters and declarations, and drop blank lines inside statement bodies. - options/window: the file-local relase_button is renamed unselect_others to avoid clashing with the shared one and to fit a single hover helper. - menu/quest: free the my_itoa/my_strcat intermediates that modify_quest leaked; string helpers come from my_str.h (C-H1).
- events: declare handlers through events.h (C-H1), make go_to_raycasting static, and flatten zoom/event_menu with early returns. - free/free2: introduce free.h for the cross-file destructors, make the per-stage helpers static, hoist loop counters, split the oversized do_free and free_params, and put one free per line. The destroy sequences are preserved as-is. - actions: relase_button now comes from display.h instead of a local forward declaration.
Replace the SFML 2 boolean shim (sfBool/sfTrue/sfFalse) with the standard <stdbool.h> bool/true/false across the codebase and drop the hand-rolled enum, which clashed with stdbool and violated the naming rules. Convert the function-pointer tables to C99 designated initialisers (.field = value), removing the 17 GNU-extension warnings, and de-indent the forward typedefs to column 0. Rename the mixed-case enum constants to UPPER_SNAKE_CASE (none -> NONE, Neutral/Attacking -> NEUTRAL/ATTACKING, npc None/Talking -> NPC_NONE/NPC_TALKING). The build is now warning-free and every header is Banana-clean.
Bring the (dormant) network layer to zero findings and close the remaining hazard: - receive_clients_infos validated nothing before indexing clients[] with a value read from the socket. The per-packet decode is now isolated in read_client, which rejects any index outside [0, MAX_CLIENTS). - connect_to_server leaked the network struct on a failed connect or selector wait, and never checked its own allocation; both are fixed. check_connection is now static and connect_to_server is declared in network_functions.h. - my_str_to_word_array: remove the comma operators from the for increments (C-L1) and split out word_len to stay under the line limit. With this the whole repository passes Banana with zero code and zero repository infractions, and the build is warning-free.
The single CFLAGS variable mixed compile and link flags, so the implicit compile rule received the -L/-l flags and clang warned (unused-command-line-argument) for every object. Linker flags now live in LDFLAGS/LDLIBS used only at link time, and the duplicated -lcsfml-system / -lcsfml-audio entries are removed. The build is now completely warning-free.
build: migrate from SFML to CSFML 3.x API
The MENU_FLAGS / START_FLAGS / PARAMS_FLAGS function-pointer tables were defined as `static const` in headers, so every translation unit that included them got its own private copy referencing all the dispatch functions. That forced any object file to be linked against the whole program, which made isolated unit tests impossible. The tables are now declared `extern const` in the headers and defined once in flags.c. Behaviour is unchanged, but a single object file can be linked on its own again — verified by linking get_map.o + my_getnbr.o in isolation. This unblocks the upcoming criterion test suite.
Add a `tests_run` target and a first criterion suite covering the correctness and memory-safety fixes that can be exercised without a window: - my_getnbr / my_itoa (INT_MIN..INT_MAX) / my_strcmp / my_strncmp / my_strlen - raycast_cell: out-of-range indices return "wall" - set_vector_speed: normalises and survives the zero vector (no hang) - get_map: valid, separator-less, missing and truncated inputs 10 tests, all passing. Coverage flags are intentionally left out for now: several source files share a basename across create/ and display/, which makes gcov collide; that is a follow-up once the build emits per-directory objects.
test: add criterion unit-test suite
refactor: move dispatch tables out of headers
Add `debug` (builds the game with -g3 -fsanitize=address,undefined), `tests_asan` (runs the criterion suite under ASan + UBSan) and `valgrind` (runs it under valgrind, for Linux/CI — valgrind is unavailable on Apple Silicon). The tests now free what they allocate so leak detection is meaningful. Verified: `make tests_asan` reports 10/10 passing with no AddressSanitizer or UndefinedBehaviorSanitizer findings, confirming the bounds, parsing and vector fixes are memory- and UB-clean on the exercised paths.
The workflow only checked repository size and coding style; it never built the code or ran the tests, and it predates the CSFML 3 migration. Add a build_and_test job that installs the SFML 3 / CSFML 3 toolchain (built from source and cached, since the Ubuntu packages are still CSFML 2), compiles the project with `make`, and runs the criterion suite with `make tests_run`. The mirror step now waits on both the coding-style and build/test jobs so a broken build is never mirrored. The existing size and coding-style jobs are unchanged.
The check_coding_style job pulled ghcr.io/epitech/coding-style-checker, which no longer exists (manifest unknown) — the job had been failing on infrastructure, not on the code. Epitech replaced that tool with Banana. Add a coding_style job that installs Banana from the epitech PPA, runs the epiclang plug-in over the sources (filtered to project files; CSFML 3 headers are fetched so the includes resolve) and runs banana-check-repo for the repository rules. The mirror now waits on this job and on build_and_test.
banana-check-repo scans the working directory, so cloning CSFML into the checkout made it flag every CSFML header as an invalid file name (C-O4). Clone the header tree into $HOME instead, leaving the repository tree clean for the repository-rule check.
The push_to_mirror job ran on every push and failed with "no path specified" whenever vars.MIRROR_URL is unset (any fork without the Epitech mirror configured). Guard it on a non-empty MIRROR_URL so it is skipped instead of failing; it still runs once the mirror variable and SSH key are set.
build: add sanitizer and valgrind test targets
ci: build and test the project in CI
game->menu is a stack of screens encoded in base 10 (menu = menu * 10 + N pushes a screen, menu /= 10 pops). Every call site read or wrote that as raw decimal arithmetic (menu % 10 == 6, (menu / 10) % 10 != 6, menu * 10 + 6, ...), which was unreadable and error-prone. Introduce menu.h/menu.c: named state constants (MENU_START, MENU_GAME, MENU_WIN, MENU_PAUSE_*, ...), transition helpers (menu_push, menu_pop, menu_set_tab) and predicate helpers (menu_in_overworld, menu_pause_open, menu_tab_music, menu_won, ...). Each helper reproduces the original expression exactly, so behaviour is unchanged — this is a pure readability pass. The remaining bare comparisons are encoding thresholds inside already-named functions. Verified: build warning-free, full Banana scan still 0 findings, the criterion suite still passes 10/10.
refactor: name the game->menu state machine
Add 10 criterion tests: - test_menu: exercises the menu transitions (push/pop/set_tab) and every predicate (overworld, hud, pause, pause tabs, settings tabs, win/lose), locking in that the named helpers behave like the old decimal arithmetic. - test_str_more: my_strcat, my_strndup, my_str_to_word_array, and my_put_nbr / my_float (captured through a temp fd) across the full int range. Suite is now 20 tests, all passing under `make tests_run` and `make tests_asan` (no ASan/UBSan findings).
test: cover the menu state machine and more helpers
The server address/port, the window framerate cap and the default resolution/fps/volume were scattered as bare literals across connection.c, start_game.c and create/window.c. Collect them in config.h (SERVER_IP/PORT/TIMEOUT, DEFAULT_FPS, FPS_CAP, DEFAULT_VOLUME, DEFAULT_WIDTH/HEIGHT/BPP). Values are unchanged, so behaviour is identical; the hard-coded server IP flagged in the audit is now in one named place ready to be made configurable when networking is hardened.
refactor: group hard-coded settings into config.h
sfKeyboard_isKeyPressed polls the OS directly, which on macOS returns false unless the app has the "Input Monitoring" permission — so movement, attack, interact and Escape silently did nothing on a fresh machine. Add an input module that maintains a per-key pressed state from the window's KeyPressed/KeyReleased events (and clears it on focus loss), and replace every sfKeyboard_isKeyPressed call with is_key_held(). The event loops (2D, raycasting and the intro dialog) now feed input_handle_event, and Escape is handled purely from the event. This only needs window focus, so the game is playable everywhere without any system permission. Build warning-free, full Banana scan back to 0, 20/20 tests pass under ASan/UBSan.
feat: event-driven keyboard input (no Input Monitoring needed)
The automated include insertion for menu.h / input.h dropped them right after the <system> header and before the project headers in five files. Move them to the end of the project-include group so the ordering is consistent (system headers, then project headers). Include-only change, no behaviour difference.
chore: group project includes after system headers
Collaborator
Author
|
@DoctorPok42 If you ever wanna have a look into this |
DoctorPok42
approved these changes
Jun 20, 2026
Collaborator
Author
|
En avant pour la mise à jour ! |
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
2 participants
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.