Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Warn about unused return values #125

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

AntonioND
Copy link
Member

This can be quite useful to detect places in your code where you aren't checking for errors, but it can also be quite annoying.

There are some functions where this is perfectly reasonable, like oamAllocateGfx(), whose only purpose is to return an address.

In other cases it's a bit less clear, like glTexImage2D(). In this case, the function can fail if there isn't enough VRAM left for your texture, and you should definitely check for that, but this will cause many warnings in code ported from old libnds versions (and it would make it harder to have code that works with devkitARM and BlocksDS).

This PR is just to get feedback, for now. I will definitely apply the changes to functions like oamAllocateGfx() but I want to know how people feel about the rest.

@AntonioND
Copy link
Member Author

This commit adds warnings to a few functions that can cause memory leaks if their results aren't cleaned:

5cacd94

I've now split this PR into two commits. One of them adds warnings to nitroFSInit() and fatInit(). The idea is that the player should always get an error message with useful information if they fail. The game shouldn't wait until fopen() fails, for example.

The other one adds warnings to lots of functions, but that may not be that useful.

In the case of the videoGL functions, adding the warning may show new users of BlocksDS that they can check for errors now, which would be nice. But maybe that should just be added to the documentation of porting projects from devkitARM.

@AntonioND AntonioND force-pushed the warn_unused branch 2 times, most recently from 91d3f4b to ea21525 Compare November 6, 2024 02:01
@AntonioND
Copy link
Member Author

I've applied what was the first patch in this PR: e82ae51

I'm still thinking about the last one, I will probably apply a reduced version of it because the current one really makes a lot of code generate warnings. Particularly, code that has been ported from devkitARM, where the GL functions don't return error codes so the code doesn't check it. Also, it isn't possible to make compatible code with devkitARM and BlocksDS if I expect people to always check the error code (of course, with devkitARM you never know if a texture has been loaded, but that's another topic...).

@asiekierka
Copy link
Contributor

it isn't possible to make compatible code with devkitARM and BlocksDS

BlocksDS has already broken compatibility in a bunch of places.

I think at some point you'll just have to cut the losses - either by breaking compatibility and providing documentation, or by using a mechanism like the proposed BLOCKSDS_STRICT, or by having a libnds2 library which libnds acts as a compatibility wrapper on top of (think sdl1.2-compat and `sdl2-compat), or...

@AntonioND
Copy link
Member Author

Well, in this case I'm thinking about code ported from devkitARM to BlocksDS. It's fair to add new warnings in a couple of places, but not giving a warning on every single GL function call...

@AntonioND
Copy link
Member Author

I could have a WARN_ALL_UNUSED_RETURNED_VALUES build option (name TBD) to enable this, though.

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.

2 participants