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

DINGUX: Only evaluate the toupper() macro argument once #3961

Merged

Conversation

dwatteau
Copy link
Contributor

@dwatteau dwatteau commented Jun 4, 2022

There are currently some tolower() or toupper() calls where its argument has some side effects:

git grep '\btolower(' | egrep -e 'tolower\((.*)\+\+(.*)\)' -e 'tolower\((.*)\((.*)\)'
git grep '\btoupper(' | egrep -e 'toupper\((.*)\+\+(.*)\)' -e 'toupper\((.*)\((.*)\)'

The Dingux/OpenDingux port currently defines its own toupper() macro "to avoid triggering an assert in uClibc dingux library", but it currently does so this way:

#define toupper(c) (((c & 0xFF) >= 97) && ((c & 0xFF) <= 122) ? ((c & 0xFF) - 32) : (c & 0xFF))

and so if (c) has side effects, they're going to happen more times than intended on this platform.

Fixing the toupper()/tolower() callers is a solution, but sometimes the error comes back, so making a safer macro shouldn't hurt.

The following PR does so with a GCC statement expression. That's a GCC-ism by definition (clang also accepts it), but AFAICS this port is only ever built with GCC anyway, and we're already in the middle of a hack in a DINGUX ifdef. __extension__ should make -pedantic happy.

I don't have any device to test this, though. I see that there was some OpenDingux port activity in 2021.

toupper() callers should avoid putting a side-effect expression there, but
in practice it sometimes happen, so making the macro safer can't hurt.

This uses a GCC statement expression, but this port only uses GCC
anyway.
@dwatteau dwatteau force-pushed the fix/dingux-tolower-macro-only-evaluate-once branch from 7ffc795 to 65bd8d1 Compare Jun 4, 2022
@dwatteau dwatteau changed the title DINGUX: Only evaluate the tolower() macro argument once DINGUX: Only evaluate the toupper() macro argument once Jun 4, 2022
@sev-
Copy link
Member

@sev- sev- commented Jun 12, 2022

Thank you

@sev- sev- merged commit 4567f1b into scummvm:master Jun 12, 2022
8 checks passed
@dwatteau dwatteau deleted the fix/dingux-tolower-macro-only-evaluate-once branch Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants