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

expr: increase size of varname #12305

Merged
merged 1 commit into from Nov 22, 2023

Conversation

SuperSamus
Copy link
Contributor

1 dollar sign + 1 negative sign + 10 characters for the longest number + 1 null terminator = 13 characters.
Fixes compilation error with certain flags.

@Pokechu22
Copy link
Contributor

Looks good to me. You might want to send this upstream to https://github.com/zserge/expr too (though I'm not sure how actively that's maintained).

CC @smurf3tte, @TryTwo since you originally worked on conditional breakpoints and are more familiar with this code.

@SuperSamus
Copy link
Contributor Author

It's 4 there, for some reason.

@Pokechu22
Copy link
Contributor

Looks like that was changed in the first force-push on #9334 (https://github.com/dolphin-emu/dolphin/compare/011ce338d8b190330b35a8361e161ad968a82bba..01f5d4774bff6171cfa0bf575323bfc625ee6de3). I don't see any corresponding review comments though.

It looks like the original code used sizeof(varname) - 1 in addition to using a 4-element array. Since snprintf writes "At most buf_size - 1 characters" in addition to the null terminator, that means that the original implementation only works for up to 10 macro parameters (after which it'd start assigning duplicates, I guess?). In practice, that's probably all that's ever needed, and I think neither billions of macro parameters nor a negative number of macro parameters makes sense in practice, but it's still better to be safe (and to fix the warning).

@TryTwo
Copy link
Contributor

TryTwo commented Nov 19, 2023

I'm not sure. I'd have to do some furious breakpointing in VS to make sense of it. If bumping it by one isn't expected to break anything, I guess it's okay. If you need someone to look into it, let me know.

@AdmiralCurtiss AdmiralCurtiss merged commit ab76b15 into dolphin-emu:master Nov 22, 2023
11 checks passed
@smurf3tte
Copy link
Contributor

The index in question can't be negative, so the only real effect of this change is to silence an erroneous warning.

@SuperSamus SuperSamus deleted the expr-buffer-size branch December 12, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants