Development - #18
Conversation
There was a problem hiding this comment.
Sorry @Blizzardo1, your pull request is larger than the review limit of 150000 diff characters
Signed-off-by: Adonis Deliannis <blizzardo1@blizzeta.net>
There was a problem hiding this comment.
Pull request overview
This PR updates SharpSDL3’s C# bindings with a broad set of wrapper refactors (primarily local-variable typing modernizations) and introduces/adjusts some public-surface items (exception type + docs), alongside a library target framework bump.
Changes:
- Refactors SDL_ttf wrapper code in
Ttf.cs(e.g.,varusage, explicit struct initialization inoutpaths). - Bumps
SharpSDL3target framework tonet10.0. - Adds/updates public API documentation elements (
SdlExceptiondocs,ColorspaceXML docs).
Reviewed changes
Copilot reviewed 35 out of 44 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| SDL3/TTF/Ttf.cs | Refactors SDL_ttf wrapper code; includes a version-query method with a correctness issue. |
| SDL3/SDL/Tray.cs | Updates tray entry enumeration logic; currently returns incorrect results. |
| SDL3/SharpSDL3.csproj | Updates library target framework; introduces test/build incompatibility with net9.0 tests. |
| SDL3/SDL/SdlException.cs | Introduces SdlException; XML docs currently misdescribe its scope. |
| SDL3/Enums/Colorspace.cs | Updates enum docs; includes a small typo in a summary line. |
Comments suppressed due to low confidence (3)
SDL3/SDL/Tray.cs:146
- GetTrayEntries() copies the native entries into the managed
entriesarray, but then returns a Span created fromref result(a single element containing the pointer value) andToArray()of that. This returns incorrect data and effectively ignores the copied entries.
SDL3/TTF/Ttf.cs:996 - GetHarfBuzzVersion() reads
major/minor/patchfrom the allocated pointers before callingTTF_GetHarfBuzzVersion, so it returns uninitialized data and the subsequent zero-check is unreliable.
SDL3/SDL/Tray.cs:144 - GetTrayEntries() copies the native entries into the managed
entriesarray, but then returns a Span created fromref result(a single element containing the pointer value) andToArray()of that. This returns incorrect data and ignores the copied entries.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Adonis Deliannis <blizzardo1@blizzeta.net>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adonis Deliannis <blizzardo1@blizzeta.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 44 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (22)
SDL3/SDL/Tray.cs:146
GetTrayEntriesallocates and fills anentriesarray, but then ignores it and instead constructs aSpanoverresult(the pointer variable) vianew Span<IntPtr>(ref result). This either won’t compile (missing length overload) or will return only a single element (the pointer value) rather than the actual tray entries.
SDL3/TTF/Ttf.cs:1001GetHarfBuzzVersionreads from the allocated buffers before callingTTF_GetHarfBuzzVersion, somajor/minor/patchwill be uninitialized/garbage. Call the native function first, then read the values, and ensure the allocations are freed viatry/finally.
SDL3/TTF/Ttf.cs:1692GetTextSubStringsForRangecan dereferencepSubStringseven when it isnint.Zero(it only logs and continues). Also,pCountis allocated withMalloc(uninitialized), so on failurecountmay become garbage. InitializepCountto 0 and return an empty array when the native call fails or returns no substrings.
SDL3/SDL/IO.cs:45GetIoSizethrows when the size is 0, but SDL returns a negative value on failure and 0 is a valid size (e.g., an empty stream). This will incorrectly throw for empty files/streams.
SDL3/SDL/IO.cs:60GetIoStatuscurrently throws when the returned status value is 0, butIoStatus.Readyis defined as 0. This makesGetIoStatusunusable on the most common path. It should return the status directly (and let callers decide how to handleIoStatus.Error).
SDL3/SDL/Tray.cs:146GetTrayEntriesallocates and fills anentriesarray, but then ignores it and instead constructs aSpanoverresult(the pointer variable) vianew Span<IntPtr>(ref result). This either won’t compile (missing length overload) or will return only a single element (the pointer value) rather than the actual tray entries.
SDL3/TTF/Ttf.cs:1001GetHarfBuzzVersionreads from the allocated buffers before callingTTF_GetHarfBuzzVersion, somajor/minor/patchwill be uninitialized/garbage. Call the native function first, then read the values, and ensure the allocations are freed viatry/finally.
SDL3/TTF/Ttf.cs:1692GetTextSubStringsForRangecan dereferencepSubStringseven when it isnint.Zero(it only logs and continues). Also,pCountis allocated withMalloc(uninitialized), so on failurecountmay become garbage. InitializepCountto 0 and return an empty array when the native call fails or returns no substrings.
SDL3/SDL/IO.cs:45GetIoSizethrows when the size is 0, but SDL returns a negative value on failure and 0 is a valid size (e.g., an empty stream). This will incorrectly throw for empty files/streams.
SDL3/SDL/IO.cs:60GetIoStatuscurrently throws when the returned status value is 0, butIoStatus.Readyis defined as 0. This makesGetIoStatusunusable on the most common path. It should return the status directly (and let callers decide how to handleIoStatus.Error).
SDL3/SDL/Touch.cs:51GetTouchDevicesreturns a managed copy of the array but never frees the native buffer, even though the XML docs state it "should be freed with Free". This leaks unmanaged memory on every call.
SDL3/SDL/Touch.cs:90GetTouchFingers(ulong touchId)returns a managed copy but never frees the native allocation, despite the docs explicitly saying it must be freed withFree. This leaks unmanaged memory.
SDL3/SDL/Touch.cs:111GetTouchFingers(ulong touchId, out int count)returns a managed copy but never frees the native allocation, despite the docs explicitly saying it must be freed withFree. This leaks unmanaged memory.
SDL3/SDL/Camera.cs:227GetCamerascopies the returned IDs into managed memory but never frees the native array, even though the docs state it "should be freed with Free". This leaks unmanaged memory each call.
SDL3/SDL/Camera.cs:266GetCameraSupportedFormatscopies the returned pointers into managed memory but never frees the native array, even though the docs state it "should be freed with Free". This leaks unmanaged memory each call.
SDL3/SDL/JoySticks.cs:596GetJoystickshas multiple issues: (1) it initializesList<nint>withnew List<IntPtr>(count)which is a generic type mismatch and may not compile; (2) it usesi * nint.Sizeas the element stride while reading 32-bit values viaReadInt32, which will produce wrong offsets on 64-bit; and (3) it never frees the native array even though the docs say it must be freed withFree.
SDL3/SDL/Touch.cs:51GetTouchDevicesreturns a managed copy of the array but never frees the native buffer, even though the XML docs state it "should be freed with Free". This leaks unmanaged memory on every call.
SDL3/SDL/Touch.cs:90GetTouchFingers(ulong touchId)returns a managed copy but never frees the native allocation, despite the docs explicitly saying it must be freed withFree. This leaks unmanaged memory.
SDL3/SDL/Touch.cs:111GetTouchFingers(ulong touchId, out int count)returns a managed copy but never frees the native allocation, despite the docs explicitly saying it must be freed withFree. This leaks unmanaged memory.
SDL3/SDL/Camera.cs:227GetCamerascopies the returned IDs into managed memory but never frees the native array, even though the docs state it "should be freed with Free". This leaks unmanaged memory each call.
SDL3/SDL/Camera.cs:266GetCameraSupportedFormatscopies the returned pointers into managed memory but never frees the native array, even though the docs state it "should be freed with Free". This leaks unmanaged memory each call.
SDL3/SDL/JoySticks.cs:596GetJoystickshas multiple issues: (1) it initializesList<nint>withnew List<IntPtr>(count)which is a generic type mismatch and may not compile; (2) it usesi * nint.Sizeas the element stride while reading 32-bit values viaReadInt32, which will produce wrong offsets on 64-bit; and (3) it never frees the native array even though the docs say it must be freed withFree.
Signed-off-by: Adonis Deliannis <blizzardo1@blizzeta.net>
No description provided.