Skip to content

Conversation

@JoeOsborn
Copy link
Contributor

This fixes a number of format, initialization, unused variable, etc warnings under both emscripten and GCC.

@warmenhoven
Copy link
Collaborator

Hm, I'm not a super huge fan of changing code in deps/ unless it's really necessary. Right now the SPIRV-Cross code happens to exactly match a particular commit from the upstream repo and keeping it that way will make future updates easier. I feel similarly about the rest of the code in deps/; if it's necessary to get something working then yeah that makes sense (and hopefully get that upstreamed) but if it's just to satisfy a nitpicky compiler then it makes me sad.

Not sure about the one in glslang, whether that fixes a legitimate bug or not. If it does, that sounds good to me (along with an upstream PR).

@JoeOsborn JoeOsborn marked this pull request as draft October 31, 2025 16:35
@JoeOsborn
Copy link
Contributor Author

Yeah, I need to revisit this since it also seems to have caused some regressions.

Except for deps and the parts that are specifically pipewire includes, this
compiles under pedantic and C89 for me on Linux.  Mostly this was
fixing issues, but in pipewire's case it involved ignoring some errors.
@JoeOsborn JoeOsborn force-pushed the fix-warnings-10-30-25 branch from 2fe84aa to ba145c5 Compare October 31, 2025 16:45
@JoeOsborn JoeOsborn marked this pull request as ready for review October 31, 2025 16:45
@JoeOsborn
Copy link
Contributor Author

I fixed the issue (a wrong hex constant in runloop.h) and removed deps/ paths from the changes.

@i30817
Copy link
Contributor

i30817 commented Oct 31, 2025

I always worry about commits like this with the way that RetroArch has some 'features' which depend on bad string hygiene. For instance, did you know that in spite of RetroArch not having a way to edit a manual scanned playlist search (just refreshing the playlist\running the old search), there is a way?

Just refresh playlist once, then go to the manual scanner. The fields will be filled.

One day, one of these gcc warning commits will fix this and I will be sad. Of course, in this case, the actual remedy is to add a "edit search" button in manage playlists, but I'm sure there are more usecases i dont know about.

Press capslock to turn on the fan indeed.

@LibretroAdmin
Copy link
Contributor

Can you check if this unintended functionality you can get with playlists still works with this PR? If so, the road is clear to merge this, right?

@i30817
Copy link
Contributor

i30817 commented Nov 5, 2025

this commit doesn't affect the manual scanner so it doesn't matter for that.

@LibretroAdmin
Copy link
Contributor

What will these pragma gcc statements do for non-GCC compilers?

@JoeOsborn
Copy link
Contributor Author

JoeOsborn commented Nov 8, 2025

Clang seems to ignore them or honor them depending on which pragma specifically. Are any of the older/weirder compilers tested through GitHub CI?

@LibretroAdmin
Copy link
Contributor

You'll need to rebase this PR now

@LibretroAdmin
Copy link
Contributor

0a253e3
OK, rebase this PR again. I backported all the warning fixes that weren't reliant on pragma push/pop directives. This way we can get an easier handle on how to proceed with this PR

@LibretroAdmin
Copy link
Contributor

This needs a rebase still

@JoeOsborn
Copy link
Contributor Author

I will, work is busy lately but I’ll try and find some time next week or in December. This could be closed for now if you want it off your queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants