Migrate the C codebase to C++20 - #793
Open
carroarmato0 wants to merge 86 commits into
Open
Conversation
…oveRetro#762) * feat: full alpha transparency support for theme colors, 3-slicing for pill sprites * feat: option for a half-transparent top/bottom curtain on game switcher (helps when using transparent pills)
Reworked the nextui and minarch makefiles to build objects per-file and link with g++, which is what lets us mix C and C++ from here on. C files stay on gnu99, C++ files get gnu++20, and settings moves up to gnu++20 too. No behavior change yet - this just clears the way to convert files to C++ one at a time.
…ered_map Renamed nextui.c to nextui.cpp and got it building as C++20. The main change: the little Hash helper - a lightweight two-arrays-plus-linear-scan map that did the job fine at the sizes it was used for - is now a std::unordered_map, which we get for free now that this is a C++ TU. The rest was just making the existing C compile cleanly as C++: static_casts on the void* array reads, casts on malloc/Array_pop, const-correct label arrays, and named SDL_Rect temps instead of taking the address of a compound literal. Builds clean on desktop and behaves exactly like main - same behavior headless too, including a pre-existing font-under-dummy-driver crash that predates this change.
The tg5040 cross-compiler is still gcc 8.3, which knows -std=gnu++2a but not the newer -std=gnu++20 spelling, so hardcoding gnu++20 broke the device build. Probe the compiler at make time and use gnu++20 when it's available (desktop gcc, and the gcc 13/14 toolchain once we get there) or fall back to gnu++2a otherwise. Same language either way for what we use. Builds clean on desktop and cross-compiles for tg5040.
The background/thumbnail image loaders and the pill-animation worker use a hand-rolled multithreaded task queue with a latent memory-corruption bug: a stray write that smashes the stack (and even the dynamic linker's loaded-lib list). It was benign under the old C build but turns fatal once the file is compiled as C++ - the different codegen/layout shifts the write onto something that matters. It crashes a worker thread on game launch/exit; ASan masks it (timing/layout), which is how we cornered it. For the C++ port POC, gate those subsystems off behind NEXTUI_POC_NO_WORKERS: don't spawn the three worker threads, no-op the image loaders (folderbg/thumb surfaces stay NULL and the existing draw guards fall through to a clear), and set the selection pill's final position directly instead of queueing it for the animation worker. No thumbnails, box art, backgrounds or pill animation for now. Confirmed on-device (tg5040/Brick): boots, browses, launches and quits games cleanly - the crash is gone. These subsystems get reintroduced later with a proper thread-safe implementation.
The old IntArray was a fixed int[27] with an unchecked push - fine in practice since it only ever holds the A-Z+# alpha buckets, but a silent overflow waiting to happen if that assumption ever changed. std::vector drops the cap and the hand-rolled new/push/free. alphas stays a heap pointer for now so we don't have to touch how Directory is allocated.
Replaces the hand-rolled grow-by-doubling void** array with a std::vector under the hood. The public Array_* API and the typed wrappers (EntryArray/StringArray/DirectoryArray/RecentArray) stay exactly the same, so call sites only change ->items[i] to ->v[i] and ->count to ->count(). Semantics preserved on purpose: - Array_free stays container-only; the typed *Array_free wrappers keep freeing their elements first (mixing those up is a leak or a double free, so they're deliberately left untouched). - Array_yoink still moves the pointers over and frees only the source container. - Array_remove now no-ops on a missing item instead of walking off the end like the old while-loop did - strictly safer, same happy path. - EntryArray_sort keeps the same case-insensitive, not-stable ordering, now via std::sort instead of qsort.
Hash_set copies its value into a std::string now (since the Hash became an unordered_map), so the strdup() the two callers passed in was never freed - a small leak on every config/map line parsed. The strdup was a leftover from the old Hash that stored the raw pointer. Just pass the value straight through.
Swaps sprintf/strcpy for snprintf on every target that's a stack char array, so a long ROM name or path from the SD card truncates instead of smashing the 256-byte buffer. sizeof(buf) is provably right here since these targets are only ever declared as arrays. Also rewrote getUniqueName - it built '<name> (<tag>)' with a four-step strcpy-via-moving-pointer dance and computed a filename it never used. That's just one snprintf now. Left alone for a careful follow-up: the writes that target a pointer or a function param (cue_path, the tmp cursor, cmd, etc.) - sizeof would be the pointer size there, so those need their real buffer size worked out per caller.
Finishes the string-safety pass - nextui.cpp now has zero raw sprintf/strcpy/strcat. These were the trickier ones sizeof can't handle: - Function params (cue_path, m3u_path, disc_path): every caller passes a char[256], so they're bounded to 256 with a comment noting why. - The tmp write-cursor pattern (build a path prefix, then strcpy a readdir name or an extension onto the tail): bounded to the space actually left in the parent buffer, i.e. SIZE - (tmp - parent). The readdir-name appends were the real overflow risk - a long filename could run past the 256-byte path buffer. All behaviour-preserving for normal-length paths; a too-long path now truncates instead of corrupting the stack.
First of the void*->T* cleanups. The recents list is now a real std::vector<Recent*> instead of a void*-backed Array, so the 8 static_cast<Recent*> at the use sites are gone and the compiler knows the element type. Mechanical, behaviour-preserving: push_back/insert/pop_back/erase for the old Array_* calls, std::swap for the bump-to-top, and the typed RecentArray_indexOf/free helpers now take the vector by reference. The generic Array_* stay put - stack and entries still use them until their turn.
Second void*->T* cleanup. The nav stack (and pathToStack, which builds it) are now std::vector<Directory*>, dropping the 4 static_cast<Directory*>. pathToStack returns the vector by value; DirectoryArray_pop/free take it by reference. 'top' still just aliases the last element (stack.back()), so the pop-then-reassign ordering is unchanged.
Third void*->T* cleanup. The two local path lists (parent_paths in getCollections and last in loadLast) are std::vector<char*> now. Dropped the unused StringArray_indexOf; StringArray_free takes the vector by ref and frees each strdup'd string before clearing. Only Entry is left on the void*-backed Array now.
The last and biggest void*->T* cleanup, and it lets the hand-rolled Array type go away entirely. - Directory now holds its entries and alphas by value (std::vector), so Directory is allocated with new/delete instead of malloc/free. new Directory() value-initialises, so the scalar fields still start zeroed. - The nine get* builders (getRoot/getRoms/getEntries/getCollection/ getDiscs/getRecents/getCollections/getQuick*) return std::vector<Entry*> by value; quick/quickActions are by-value vectors too. - EntryArray_indexOf/sort/free take the vector by reference; the merge spots that used Array_yoink now splice with insert()+clear(). - Every static_cast<Entry*> is gone (58 casts removed across the whole Array->vector effort). With nothing left using it, the generic Array struct and its new/push/pop/unshift/remove/reverse/free/yoink are deleted. The whole crusty hand-rolled container layer is now std::vector.
Phase B kicks off common/core/ - a modern-C++ utility layer the other binaries can share (settings now, minarch once it's C++). core/str.h has copy/format/append that take the destination by reference to a fixed-size char array, so N is deduced at compile time. The nice part: you physically can't call them on a decayed char* - that's a compile error, not the silent 8-byte 'size' you'd get from snprintf(ptr, sizeof(ptr), ...). So they can't overflow and can't be mis-sized. There's a desktop self-test (tests/str_test.cpp) covering copy/format/append incl. truncation. Adopted them in nextui.cpp: the 63 snprintf(buf, sizeof(buf), ...) array sites became core::format(buf, ...) and the one bounded strncat became core::append. That it all compiles is itself a proof that every one of those targets really is a fixed array - if any were secretly a pointer, the template wouldn't compile. The pointer/cursor/param string writes stay as plain snprintf with their real runtime size. Header-only, so no makefile wiring - -I../common already covers it.
Starts the minarch robustness pass (Phase E), in plain C - no C++ conversion, since these files are emulation-critical and full of C-isms (designated inits etc). Same approach as the nextui string work: - Array and struct-field targets (game.path/name/alt_name/m3u_path, local path buffers) get snprintf(T, sizeof(T), ...); dropped the redundant (char*) casts on the game.* fields. - The tmp m3u-path cursor is bounded to the space left in m3u_path. The dangerous ones here were the filename copies - strcpy(game.name, strrchr(path,'/')+1) into a 128-byte field would overflow on a long ROM name. Now it truncates.
Config path/key/value buffers now snprintf with sizeof. The param and cursor cases handled explicitly: Config_getPath's filename is MAX_PATH at every caller; the default.cfg path cursors are bounded to the space left in their path buffer. The calloc(strlen+1)+strcpy dup became a plain strdup.
Finishes the minarch robustness pass - zero raw sprintf/strcpy/strcat left in the whole frontend. All in plain C (these stay .c for now). - Local + struct-field char arrays (game/core/menu fields, path buffers, cheat paths[][MAX_PATH]) -> snprintf with sizeof; const Core fields keep their (char*) cast. - Params bounded to the caller's real size: ma_saves filename is MAX_PATH; getAlias's alias is >=256 everywhere; Core_getName got an explicit out_size param (its one caller passes sizeof(core.name)) so the basename copy can't overflow core.name[128]. - The tmp path cursors are bounded to the space left in their parent. - The calloc(strlen+1)+strcpy dups became strdup - which also fixes a latent mis-size in ma_options where item->name was allocated for def->desc but copied getOptionNameFromKey()'s (possibly longer) result. Careful bit: an earlier auto-convert pass naively used sizeof() on struct-field targets, but field names like key/value are ambiguous across structs and some are char* (sizeof would be 8 -> truncation). Reverted that and classified every struct-field target by its real type.
First step of the data-driven device HAL, tg5040 only. The Brick and the Smart Pro were already 'two devices in one binary' - scattered is_brick?X:Y ternaries in ten macros (screen geometry, menu layout, a few joystick codes). Those are now two DeviceDescriptor instances, TG5040_BRICK and TG5040_SMART_PRO, selected at runtime by a single resolveDeviceModel() that replaces the four copy-pasted getenv+exactMatch blocks (there was even a maintainer TODO asking for exactly this). The geometry/button macros (FIXED_WIDTH, MAIN_ROW_COUNT, JOY_L3, ...) now read deviceModel->field. Safe because those macros were already runtime values (is_brick is a runtime global), so nothing used them as compile-time constants. deviceModel defaults to Smart Pro so the macros are valid before init, then resolveDeviceModel refines it. Behaviour-preserving. The PLAT_* facade is untouched, so nothing above the platform layer changes. This is the shape tg5050 slots into next - it becomes another descriptor rather than a forked platform.c. (Named deviceModel, not deviceInfo, to avoid colliding with settings.cpp's own DeviceInfo class.) Verified on Smart Pro: boots clean at 1280x720 (the non-brick descriptor).
Device HAL step 2. Grows DeviceDescriptor with the sysfs nodes that actually differ between platforms - the CPU-freq node, the GPU thermal zone, and the rumble GPIO. On tg5040 these are the same for Brick and Smart Pro (shared TG5040_* defines), but they're exactly where tg5050 diverges (cpu4 not cpu0, thermal_zone5 not zone2, gpio236 not gpio227), so they now live in the descriptor instead of as literals in the functions. PLAT_cpu_monitor and PLAT_setRumble read deviceModel->field. Only the values that genuinely vary went in - cpu_temp (thermal_zone0) is identical on both platforms, so it stays a literal (no point descriptor-izing a constant). The rumble *mechanism* difference (tg5040 voltage vs tg5050 level) is behavioural, not just a path, so that waits until the two platform.c bodies get unified. Behaviour-preserving on tg5040; verified on Brick (boots 1024x768, stable, rumble/perf paths unchanged).
api.h literally had a TODO over this: eight loose 'extern int's for the current render pass (currentshaderpass, currentshadersrcw/h, currentshaderdstw/h, currentshadertexw/h, should_rotate) shared between api.c, the platform GLES path (generic_video.c) and minarch (ma_video.c). Now they're one GFX_RenderState struct with a single extern, gfx_render, so the header exposes one well-named thing instead of eight. Fields renamed to the obvious short forms (pass, src_w, dst_h, ...). Purely a regrouping - same values, same cross-module writes (the struct fields stay writable), no logic touched in the GLES pipeline. api.h drops from 27 externs to 20. Verified on Brick: boots 1024x768, stable, renders fine. Note: generic_video.c is #include'd into platform.c, and make doesn't track that dep, so platform.o must be force-rebuilt (touch platform.c) after editing generic_video.c - else you get a stale-object link error.
First step of bringing the disabled visuals back, done right. The pill tween no longer runs in a worker thread. It used to: animWorker looped over frames computing the pill position, wrote it into a shared struct, handed that pointer to the main thread via a callback, and blocked on a condition variable until the next flip - with the struct written and read from two threads with no lock, and a queue-size counter decremented too late so the producer's drop-oldest ran on a stale count. That's the tangle that corrupted memory and crashed. A UI lerp doesn't need any of that. animPill now just records the start/target/frames, and advancePillAnim() - called once per main-loop iteration - steps the linear tween and sets pillRect, reproducing exactly what the old worker+callback did per frame (including hiding the text off-screen mid-slide). No thread, no queue, no cross-thread struct. The old animWorker/enqueue/animcallback/queue code is now dead; it'll be deleted in a follow-up so this commit stays focused. Image loading (backgrounds, thumbnails) is still gated off - that's next, on a proper thread-safe queue. Verified on Brick: boots, stable, no cores.
When frames<=1 (a directory change, where should_animate is false), the tween was still rendering the start position for one frame before snapping to the target - which flashed the pill at the previous directory's row (often near the top) on the way back out. Restore the instant behaviour the POC had for that case: jump straight to target, no start frame. The multi-frame slide (in-list navigation) is unchanged.
First of the image loaders returns, rebuilt right. Adds a reusable core/thread_safe_queue.h (std::mutex + condition_variable, blocking pop, drop-oldest when full) and runs the folder-background loader on one std::thread that pops requests off it. The old design was a hand-rolled SDL_mutex/SDL_cond linked-list queue whose size counter was decremented too late and whose task ownership crossed threads - that's the race the POC had to switch off. Now the correctness lives in one small tested-shaped place: the worker owns the request once it pops it, IMG_Load + SDL_ConvertSurfaceFormat only touch CPU-side pixels off the main thread, and the decoded surface is handed back through onBackgroundLoaded under bgMutex (the same mutex every main-thread draw of folderbgbmp already takes, so no torn read or use-after-free). shutdown() unblocks the worker's pop() and we join it before freeing anything. Capacity is 1: only the most recent folder's background matters; if you've already navigated on, the stale request is dropped before it loads. Thumbnails are still on the disabled path - next to migrate onto the same queue. Verified on Brick: boots, stable through navigation, no cores.
Same treatment as folder backgrounds: thumbnails now load on their own std::thread pulling from their own capacity-1 core::ThreadSafeQueue (separate from backgrounds so the two never evict each other's request). The worker checks the file exists, decodes off the main thread, and hands the surface to onThumbLoaded, which does the scaling + rounded-corner pass and stores thumbbmp under thumbMutex - the same mutex both main-thread draw sites already take. shutdown()+join on quit. Every image loader is now off the hand-rolled SDL queue and onto the tested-shaped ThreadSafeQueue; the whole NEXTUI_POC_NO_WORKERS subsystem is effectively re-enabled, rebuilt without the data race. (The dead legacy SDL queue/worker code is still in the file; a follow-up deletes it.) Verified on Brick: boots, stable through rapid navigation, no cores.
Two small fixes now that thumbnails are back: - When the highlighted entry has no box art, clear the thumbnail right away instead of queueing a load that only resolves to 'no art' a frame or two later. And only queue a load when the art file actually exists. - When the directory depth changes (moving up/down), clear the thumbnail surface and LAYER_THUMBNAIL *before* the directory transition animates. Going from a ROM folder up to the systems overview is gamelist->gamelist, so the existing 'clear when leaving a gamelist' guard didn't fire and the previous ROM's art flashed through the slide. previous_depth still holds the old depth at that point, so previous_depth != stack.size() detects the change. Verified on Brick: box art no longer lingers into the systems overview.
Now that folder backgrounds and thumbnails run on the ThreadSafeQueue + std::thread loaders and the pill animation is a plain main-thread tween, the old hand-rolled path is fully dead. This clears it out: - the SDL_mutex/SDL_cond linked-list task queues (TaskNode/AnimTaskNode, the head/tail globals, the per-queue mutexes and conds) plus their producers (enqueueBGTask/enqueueThumbTask/enqueueanmimtask) and consumers (BGLoadWorker/ThumbLoadWorker/animWorker) - animcallback + the finishedTask struct it handed back, and the frameReady/flipCond/frameMutex frame-flip handshake that fed animWorker - LoadBackgroundTask, the currentXQueueSize counters, MAX_QUEUE_SIZE, workerThreadsShutdown, and the SDL_Thread handles - the NEXTUI_POC_NO_WORKERS gate in init/cleanup and its scaffolding note AnimTask keeps only the fields animPill actually reads; init/cleanup now just create/destroy the four live mutexes (bg/thumb/anim/font) and spawn/join the two std::thread loaders. No behaviour change -- verified booting clean on the Brick with a stable launcher PID and no crash cores.
Brings the migration branch current with upstream (LoveRetro/NextUI). New upstream work pulled in: white-point colour correction + displaycal for tg5040 (LoveRetro#760), configurable font style and custom system fonts (LoveRetro#748/LoveRetro#729), usb-c card detection before boot (LoveRetro#750), the game-switcher label fix for entries without a save state (LoveRetro#751), readyResume on Quick Menu directory nav (LoveRetro#739). Two upstream commits (LoveRetro#733 cpu governor, LoveRetro#728 Rewind_init) are the same PRs our fork already carried under different SHAs; git merged them cleanly with no duplicated logic. Conflict resolution: - nextui.cpp: upstream still edits the old nextui.c, which we renamed to .cpp. Git followed the rename. The one real conflict was the recent-game preview button hints -- upstream's LoveRetro#751 changed the logic (group 0 always BACK; group 1 shows "A OPEN" when there's no save state instead of RESUME/REMOVE). Took upstream's logic, kept our C++-friendly cast style. - nextui.cpp: LoveRetro#739 added a readyResume call site into nextui.c using the old Array API (->count / ->items[]); rewrote it against std::vector to match the migrated container (top->entries[top->selected]). Also fixes a latent const-correctness bug the full clean build surfaced: the tg5040 device descriptor stores sysfs paths as const char*, but getInt/putInt/putFile took char* -- so libbatmondb (which compiles platform.c with -Werror=discarded-qualifiers) failed once its cached header forced a platform.c recompile. Those helpers only read the path string, so const-ifying the parameter is the correct fix. Verified: full clean build green for both desktop and tg5040 (nextui, minarch, settings).
Every button-hint group was passed as GFX_blitButtonGroup((char**)(const
char*[]){ ... }, ...). That GNU compound-literal argument is miscompiled by
the toolchain's g++ 8.3 -- the callee reads garbage, the pill computes as
zero-width, and nothing draws. Result: no on-screen hints (RESUME/OPEN/BACK
/SLEEP...) anywhere in the launcher.
Confirmed on device by probing one call two ways in the same frame:
named local array -> ow = 259 (correct)
compound literal -> ow = 0 (miscompiled)
The pattern came in with the C->C++ port: the original C used (char*[]){...}
(fine as C); it was cast to satisfy g++, which exposed the codegen bug. Route
the hints through a named local array instead, via a small BTN_HINTS() macro
(statement expression), and apply it to all 11 call sites. Same behaviour,
correct codegen -- hints are back, and the game-switcher RESUME/OPEN split by
save-state (from the upstream LoveRetro#751 merge) now shows correctly too.
Phase D nibble: both globals are used only inside api.c, yet were declared `extern` in api.h -- part of the god-object surface every module sees. Make them `static` and drop the externs. The LID_Context type stays public; only the instance goes private. Purely internal linkage -- no runtime change; the linker confirms nothing outside api.c referenced either symbol (all three binaries build clean).
Two related fixes: 1. skeleton/EXTRAS/Emus/tg5050/SUPA.pak/default.cfg: change supafaust_thread_affinity_ppu from 0xc (CPUs 2+3, offline on tg5050) to 0x10 (CPU 4, online). Mirrors the upstream fix in PR LoveRetro#784. 2. workspace/all/minarch/ma_core.cpp: wrap retro_load_game in try/catch in Core_load(). A core that throws during load now logs the exception and exits cleanly instead of hitting std::terminate with no context. (C++ only; upstream's ma_core.c has no equivalent guard available.)
… pak fixes New upstream features merged: - Color palettes: 18 bundled palettes + user-loadable palette dir (LoveRetro#787) - FN1/FN2/HOME can now launch user-assigned tool paks (LoveRetro#788) - Keep device awake while connected as USB gadget (LoveRetro#783) - RetroAchievements identification for custom paks and multi-system emulators (LoveRetro#775) - Brick Pro rumble limited to 2.5V (was 3.3V, too strong) - Align Brick/Pro rumble strength Conflict resolutions (C++20 branch vs upstream): - nextui.cpp: kept RAII core::SurfacePtr, took upstream THEME_COLOR7 for preview fill - minput.c: kept GFX_getFonts() and pad-> accessors, took upstream button rendering (ALT_BUTTON_TEXT_COLOR, text_color local, GFX_blitAssetColor, THEME_COLOR2 for sticks) - tg5040 platform.c: accepted conditional MAX_VOLTAGE for Brick Pro; RUMBLE_PATH not needed (branch uses deviceModel->rumble_gpio_path from DeviceDescriptor)
- nextui.cpp: restore if(lastScreen==SCREEN_GAME) branch eaten by conflict resolution and update runFnAction() to use C++20 vector accessor (entries.size() / entries[i]) - api.c/h: restore ALT_BUTTON_TEXT_COLOR as public (non-static) with extern declaration so minput.c can reference it after upstream introduced its use there - platform.c: restore MAX_STRENGTH and MIN_VOLTAGE defines lost during conflict resolution
palette.c is only consumed by the settings component (C++); no C-only tool includes it, so there is no reason to keep it as a C translation unit. - palette.h: #pragma once, <cstdint>, drop <stdbool.h> (native bool in C++), struct tag instead of typedef struct - palette.cpp: <cstdio>/<cstring> instead of C headers; extern "C" wrapper around config.h (which has no __cplusplus guard) to preserve C linkage for CFG_* calls - settings/makefile: move from gcc SOURCE to g++ CXXSOURCE; drop palette.o from the intermediate-object mv and link list - palettemenu.cpp: move palette.h include out of the extern "C" block
Finishes minarch at 100% first-party C++: the two remaining C units move from CSOURCE to CXXSOURCE in the makefile (the RA-conditional block). Both sit on the rcheevos C hashing/callback boundary, so the conversion stays conservative — allocation remains malloc/free (non-throwing) and no STL containers are introduced on the callback path. - Add extern "C" guards to chd_reader.h and ra_integration.h so their symbols keep C linkage for the 5 already-C++ consumers. - Wrap the still-C project headers (config/http/api/notification/RA helpers) in extern "C" in each .cpp, matching the ma_environment.cpp pattern; keep system/SDL/rcheevos headers outside. - chd_reader: C++ headers, libchdr includes wrapped extern "C" defensively (cdrom.h is unguarded but only supplies macros/enums). - ra_integration: drop the redundant `volatile` on ra_response_queue_count (every access is under ra_queue_mutex, so the lock provides ordering; this also clears C++20's -Wvolatile deprecation on ++/--). Builds clean on tg5040 (g++ 8.3, gnu++2a) and desktop (host g++, gnu++20).
Phase 2 (leaf tools), clean group — no SDL/compound-literals/nested functions, so these are near-mechanical. Each makefile moves from a single gcc compile+link to the settings.cpp-style mixed build: shared common/ + platform stay C (compiled to objects with gcc), the tool's .cpp is C++20, everything links through g++ -lstdc++. Still-C project headers (defines/api/utils/config, batmondb/gametimedb, msettings) are wrapped extern "C" in each .cpp. Also modernized: C++ <cXXX> headers, NULL -> nullptr. Dropped the C-only -std=gnu99 from the shared CFLAGS (it lazily leaked into the g++ command line via CXXFLAGS += $(CFLAGS)); common C now compiles at the default std, matching settings/makefile. Verified: tg5040 build clean (0 errors/warnings); nextval also builds desktop. On-device (tg5040): nextval --help, gametimectl usage, and syncsettings all run and exit 0 with correct output. batmon is a daemon — build-verified, logic unchanged.
Phase 2 (leaf tools), UI group with SDL. Same settings.cpp-style mixed makefile
(common/ + platform stay C; tool .cpp is C++20, linked via g++ -lstdc++), C
project headers wrapped extern "C".
C→C++ fixes:
- (SDL_Rect){...} compound literals -> named local SDL_Rect (the nextui.cpp
convention; g++'s handling of address-of-compound-literal is unreliable).
- (char*[]){...} button-hint arrays -> named local const char* arrays, avoiding
the g++ 8.3 miscompile that drops the literal's storage.
- clock: the four GCC nested functions (blit/blitBar/blitNumber/validate, a
C-only extension) become lambdas capturing the enclosing state by reference.
- string-literal arrays -> const char*; realloc results cast (bootlogo);
ampm_w initialized.
Residual -Wwrite-strings warnings (string literal -> char* C-API params) are
left as-is: they are inherent to the still-C common headers and already present
in bulk across nextui.cpp (~545). Verified clean builds: bootlogo tg5040; clock
tg5040 + desktop (host g++, gnu++20).
Phase 2 (leaf tools), the SDL-heavy UI group. Same settings.cpp-style mixed
makefile (common/ + platform stay C, tool .cpp is C++20, linked via g++
-lstdc++); still-C project headers (defines/api/utils/sdl, batmondb/gametimedb,
msettings) wrapped extern "C".
C→C++ fixes:
- Every &(SDL_Rect){...} compound literal -> a named local SDL_Rect. g++ in
strict C++ rejects taking the address of a compound-literal temporary
("taking address of temporary"), so this is required, not cosmetic. By-value
compound literals (drawBatteryIcon, rect = ...) become SDL_Rect{...}.
- (char*[]){...} button-hint arrays -> named local const char* arrays (the
g++ 8.3 storage-drop miscompile).
- malloc/realloc results cast; NULL -> nullptr; C++ <cXXX> headers.
- ledcontrol: explicit (int) cast on color1 in the int settings_values[]
brace-init to make the uint32_t->int narrowing intentional (-Wnarrowing).
Verified: full `make build` clean on tg5040 (g++ 8.3, gnu++2a) and desktop
(host g++, gnu++20); all 10 tool elfs produced. minput also builds desktop.
Phase 3 (leaf libraries). These build .so's whose C ABI is consumed by the battery/batmon and gametime/gametimectl binaries, so the exports must stay C-linkable: batmondb.h / gametimedb.h gain #ifdef __cplusplus / extern "C" guards, which also keeps their existing consumers (that wrap the include in extern "C") working via harmless nested extern "C". - Convert .c -> .cpp; wrap the still-C common headers (defines/utils) in extern "C"; sqlite3.h and the guarded lib header stay outside. - Makefiles split into a gcc C compile (common utils/api/platform) + a g++ C++20 compile of the library unit, linked -shared through g++ -lstdc++. - The library compile keeps -Werror; the C++ unit adds -Wno-write-strings because the exports call common C helpers taking char* (exists/touch, sql string literals) — benign but an error under C++ -Werror. Modernized internals only (nullptr in batmondb, malloc cast in gametimedb); no ABI change. Verified: full `make build` clean on tg5040; both .so's rebuilt and all four consumer elfs relink. On-device, `gametimectl list` loads the new C++ libgametimedb.so and prints play activity correctly (exit 0).
Phase 4 (shared common/ core), first sub-batch — the lowest-fan-out RA helpers, consumed only by minarch and/or settings. Both consumers already link via g++, so this just moves the units from each makefile's C-compiled list to the C++-compiled list. The common files keep C linkage (platform.c is still C and calls into them): ra_sync.h / ra_event_queue.h were already extern "C"-guarded; added guards to ra_offline.h. Each .cpp wraps the still-C project headers it includes in extern "C" (ra_integration.cpp pattern). C++ fixes required by the stricter compiler: - ra_offline.cpp: two functions used `goto` to cleanup labels across local initializations; hoisted the crossed declarations above the jumps (the labels don't use those vars). Same class as the earlier ma_saves fix. - ra_event_queue.cpp: it forward-declares LOG_note locally (to avoid api.h's transitive deps); marked that declaration extern "C" so it resolves to api.c's C symbol instead of a mangled C++ one. Verified: full `make build` clean on tg5040 (g++ 8.3) and desktop (host g++); minarch + settings relink on both.
…++20
Phase 4 (shared common/ core), second sub-batch — the rest of the low-fan-out
RA/http/notification helpers (consumed only by minarch and/or settings). Added
extern "C" guards to their headers so exports stay C-linkable for the still-C
platform.c; each .cpp wraps the still-C project headers it includes in
extern "C". Moved the units to the C++ side of the minarch + settings makefiles.
Code fixes surfaced by C++:
- http.cpp: 9 malloc/calloc/realloc results explicitly cast to their target type.
- ra_auth.cpp: calloc result cast.
- notification.cpp: the one &(SDL_Rect){...} compound literal -> a named local.
Verified: full `make build` clean on tg5040 (g++ 8.3) and desktop (host g++);
minarch + settings relink on both.
Remaining in Phase 4: the four high-fan-out core files (utils, scaler, config,
api), each compiled into 11-14 binaries.
Phase 4 (shared common/ core), third batch — three of the four high-fan-out core files, done together so each consumer makefile is edited once. Converted across all 15 consumers (9 tools, minarch, settings, nextui, both libs, and the tg5040 poweroff_next platform tool). Only api.c remains in C. Kept C linkage throughout (platform.c is still C and calls these): added extern "C" guards to utils.h / config.h / scaler.h; scaler.cpp now includes its own header so its exports keep C linkage; each .cpp wraps still-C project headers in extern "C". C++ fixes the stricter compiler required: - utils.cpp: strrchr/strstr on const args return const char* in C++ — const- correct baseName's local and cast the replaceString2 traversal. - config.cpp: reordered the NextUISettings default initializer to declaration order (C++20 requires it), and — because g++ 8.3 rejects designated initializers for char-array members — moved the six char[] string fields out of the aggregate into strcpy() calls. - scaler.cpp: bare `restrict` -> `__restrict` (C++ has no `restrict` keyword); `(void*)p + n` void-pointer arithmetic -> `(uint16_t*)((char*)p + n)`. - poweroff_next: its makefile now compiles the tool as C and utils/config as C++20, linking through g++. Verified: full `make build` clean on tg5040 (g++ 8.3, gnu++2a) and desktop (host g++, gnu++20); nextui/minarch/settings/all tools/libs relink on both. Remaining in Phase 4: api.c (the last core file, ~4.7k lines w/ ~210 compound literals).
Phase 4 complete: api.c was the last high-fan-out core file (13 consumers,
~4.7k lines). Moved to the C++ side of all 13 makefiles (8 tools, minarch,
settings, nextui; the two libs don't link api so it's just dropped there).
The only remaining common/ .c files are generic_*/displaycal, which are
#included into platform.c and stay C with the platform layer.
Kept C linkage (platform.c is still C and calls GFX_*/PAD_*/etc.): added
extern "C" guards to api.h; api.cpp wraps the still-C project headers.
C++ fixes:
- 39 address-of-a-compound-literal sites &(SDL_Rect){...} -> named local rects
(g++ rejects taking the address of a temporary). The ~84 by-value asset-rect
compound literals stay as GNU compound-literal rvalues (no address, no
miscompile). Two `dst_rect = &(SDL_Rect){...}` defaults became function-scope
locals so the pointer doesn't dangle.
- 3 (char*[]){...} button-hint literals in GFX_blitHardwareHints -> named local
arrays (the g++ 8.3 storage-drop miscompile).
- Reordered the libsamplerate SRC_DATA designated init to declaration order.
- int -> enum casts nextui surfaced without -fpermissive (LightProfile); one
float->int narrowing cast in a progress-bar rect.
Verified: full `make build` clean on tg5040 (g++ 8.3) and desktop (host g++);
every binary (nextui, minarch, settings, all 10 tools) and both libs relink.
displaycal is the last shared common/ source still compiled as C. It is only built into tg5040's libmsettings.so. Compile it with g++ under -std=gnu++2a but with -fno-exceptions/-fno-rtti so the object references no libstdc++ symbols, and keep linking libmsettings.so with gcc — the .so's NEEDED graph is byte-identical (no libstdc++ injected into a lib loaded by every process, including the still-C daemons).
Prep for converting each platform's platform.c (which unity-includes these
shared generic_*.c files) to C++20. All fixes are valid C as well, so the
files keep compiling as C for platforms not yet converted:
- explicit casts on realloc/malloc/calloc void* results (generic_platform
joysticks; generic_video cleaned/pragmas/overlay_path/pixels)
- reorder FX_Context 'effect' designated initializer to declaration order
- replace two &(SDL_Rect){...} compound-literal pointers in PLAT_flip with
named SDL_Rect locals (address-of-temporary is ill-formed in C++)
No behavior change; tg5040 C build verified green.
Prep for renaming platform.c -> platform.cpp. Valid C as well (no behavior change), so the files keep building as C until the makefiles switch: - reorder DeviceDescriptor initializers (joy_l4/r4/menu_alt before joy_plus/minus; rumble_gpio_path before gpu_speed_fixed) and the WIFI_connection initializer to declaration order (C++ rejects out-of-order designators) - PLAT_getCurrentTimezone: return NULL not false on failure (char*, not bool -- harmless 0==NULL in C, a type error in C++) desktop/platform/platform.c already compiles clean as C++; no change.
Renames each platform's platform.c to platform.cpp and compiles it with g++. The four shared common/generic_*.c files are textual #include fragments of platform.cpp, so they now build as C++ too (no standalone C compile remains); they keep the .c extension as include-only fragments, per route (a) -- the GLES pipeline in generic_video.c is compile-clean but otherwise untouched. - 9 tool makefiles + settings: compile the platform TU with $(CXX)/$(CXXFLAGS) instead of $(CC)/$(CFLAGS) - nextui, minarch: move the platform TU from CSOURCE to CXXSOURCE (the %.cpp pattern rule + vpath already cover it) - libbatmondb, libgametimedb: drop the platform compile entirely -- that object was compiled but never linked into the .so (dead build step) - platform.cpp wraps <msettings.h> in extern "C" (a C library header without its own guards) so libmsettings' symbols resolve with C linkage Builds green on tg5040, tg5050, and desktop. On-device runtime validation across the three handhelds still pending.
msettings is the settings-persistence shared library loaded by every process. Compile it with g++ (-fno-exceptions/-fno-rtti, -Wno-write-strings for the benign string-literal->char* helper calls) but keep linking libmsettings.so with plain gcc, so its dependency graph is unchanged -- no libstdc++ injected into a lib loaded everywhere. Verified on all 3 platforms: NEEDED list identical and SetVolume/SetBrightness/... still exported unmangled (C linkage), so every consumer links. - wrap the TU's own <msettings.h> include in extern "C" so the definitions keep C linkage (else the .so would export mangled names and break the ABI) - (Settings*) cast on mmap() results (tg5040/tg5050) - explicit (unsigned long) cast to avoid a narrowing conversion in a braced initializer (tg5040 SetRawBrightness)
The last first-party C daemons. These are standalone binaries (not the shared libmsettings.so), so linking libstdc++ is fine and consistent with how the tools link. - keymon (tg5040/tg5050): compile with g++; wrap <msettings.h> in extern "C" so libmsettings' symbols resolve with C linkage - rfkill (tg5040): compile+link with g++ - poweroff_next (tg5040): move the tool TU from the C compile to the g++ one (the makefile already linked common/utils+config as C++ through g++) Builds green on tg5040 and tg5050.
Collaborator
Author
|
Looks like build-core (tg5050, fceumm) fails due to an upstream change, doesn't look like it has something to do with my changes |
The C++20 migration PR must not change behavior. Commit 8f873ec had slipped in a tg5050-specific crash workaround (supafaust_thread_affinity_ppu 0xc->0x10, the LoveRetro#785/LoveRetro#784 fix) that belongs in its own change, not this migration. Restore it to upstream's 0xc so the PR stays behavior-neutral.
|
@carroarmato0 FYI the fceumm and mGBA builds have been fixed on the main branch, so if you rebase it should pass CI. |
Collaborator
Author
Thanks for the notice. I think I will update the branch once the H700 stuff lands as I will need to look at how that will influence the hardware abstraction to avoid repeated refactoring |
|
Otherwise I like the approach, and I don't see anything obviously wrong with it. Still, I notice that some header files have been updated with #ifdef __cplusplus
extern "C" {
#endifWhile others, such as msettings.h for instance were not, and they are wrapped in |
Brings in the fceumm/mGBA core build fixes (LoveRetro#799) that were failing CI, the RetroArch cheat-format support (LoveRetro#798), the RetroAchievements memory region timing fix (LoveRetro#801), CI action bumps, and the PolyForm Noncommercial 1.0.0 licence transition (LoveRetro#806). Conflicts resolved: - ma_cheats.c/.cpp: kept our explicit `return success` (the C original fell off the end of Cheats_load, which is UB in C++) and took upstream's new Cheats_apply(). Cast get_memory_data()'s void* explicitly — C++ has no implicit void* conversion. - ma_core.c → ma_core.cpp: applied upstream's "skip cheats with no code" guard on top of our try/catch around cheat_reset/cheat_set. - ma_runframe.c → ma_runframe.cpp: added the per-frame Cheats_apply() call. It writes to core RAM from the frontend and cannot throw, so it sits outside the exception guard that wraps core.run(). Builds clean on tg5040, tg5050 and desktop.
Collaborator
Author
|
@apommel At this point the wrappers are mostly dead weight anyway. So yes, I'll move the guards into the headers and drop the wrappers at the call sites. One exception I'd keep: rcheevos and libretro-common are git cloned by the makefile at build time, and Last thing, more a note than a question: marking the internal headers |
Reviewer feedback on LoveRetro#793: some headers carry their own `#ifdef __cplusplus / extern "C"` guard while others are wrapped in `extern "C" { … }` at every include site. The inconsistency was migration scaffolding, not design. Each step of the port converted one group of translation units, so at every intermediate commit some callers were still C; wrapping locally was what kept each commit building on its own. Nothing first-party is compiled as C any more (CSOURCE is empty in every component makefile, and the four generic_*.c files are textually included into platform.cpp), so the wrappers are dead weight. Move the guard into the header in every case where we own the header: - msettings.h (tg5040/tg5050/desktop). This is the one that mattered: it is the public header of a .so with a C ABI that third-party paks link against, and msettings.cpp had to wrap its own header to keep the exports unmangled. - minarch's ma_*.h, common/device.h, and tg5040's platform.h (the only copy declaring anything with external linkage). - Drop the wrappers that never did anything: defines.h is macros only, sdl.h is SDL's own already-guarded headers. Include-site wrappers stay only where we cannot patch the header: rcheevos and libretro-common are cloned by the makefile at build time, so a guard added to streams/file_stream.h or libchdr/cdrom.h would be wiped on a clean build. Those two sites now say so. C++-only headers (palette.h, core/str.h, core/thread_safe_queue.h) deliberately get no guard. Where a guarded header includes another guarded header the blocks nest, which is well-defined and matches what the call-site wrappers already did. No symbol linkage changes: libmsettings.so still exports InitSettings and friends unmangled, and the binaries still import the unmangled names. Verified with nm, and builds clean on tg5040, tg5050 and desktop.
The rule that copies msettings.h into PREFIX_LOCAL had no prerequisites, so once the file existed it was never refreshed. Editing the real header left a stale copy in /var/tmp/nextui/include, and the desktop build failed to link with mangled references to InitSettings/GetHDMI while the source header said otherwise. Depend on the platform's msettings.h so the copy is remade when it changes.
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.
This converts all of NextUI's own C sources over to C++20. It's purely a language and build change, there are no behavior changes and no new features. The goal is to get the C++ base in place so it can be built on later (RAII-based memory-safety cleanups and so on) without the fork drifting away from upstream.
Goal:
It's not an endgoal, but rather an enabler. Getting every first-party files compiling as clean C++ so the real wins (memory-safety, RAII, better structure) become possible without rewriting behavior first.
Once that's in place, we can look into making it object oriented, add abstraction for making it easier to port to other devices, cleanup, consistency and improved maintainability.
Just about everything first-party is covered: the shared common/ core (api, config, utils, scaler, http, notification, the RetroAchievements pieces, displaycal), nextui, all of minarch, settings, the tool paks, the two small db libs, the platform HAL (each platform.c became platform.cpp, along with the generic_* files that are #included into it), libmsettings, and the keymon/rfkill/poweroff_next daemons.
The conversion is done file by file: rename .c to .cpp and link everything through g++. The C ABI is kept intact — api.h, the PLAT_* layer and msettings.h stay behind extern "C" so libmsettings and friends still export unmangled symbols and nothing downstream had to change. The shared libraries are compiled with -fno-exceptions/-fno-rtti and still linked with gcc, so libmsettings.so doesn't pull in libstdc++ even though its source is C++ now.
It builds on the tg5040 cross-compiler (gcc 8.3, so the makefiles fall back to -std=gnu++2a where -std=gnu++20 isn't recognised), on tg5050, and on the native desktop target. I packaged a MinUI.zip and flashed it to all three devices (Brick, Smart Pro, Smart Pro S) — brightness and volume, display colours, wifi, launching a game, and sleep/power all check out.
Happy to adjust or split this up however works best for review.