Skip to content

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Nov 12, 2025

Previously nargs was not reset to 0 for subsequent predicates. If those predicates had the same number or more arguments, this was not noticeable. However, if the subsequent predicates had less arguments, this lead to out-of-bounds access causing undefined behavior.

This PR consists of two commits:

  • first one, fixing the bug and adding a test which previously triggered it
  • second one, refactoring the predicate handling by moving it to a dedicated handlePredicate method to make it a bit easier to understand

If you want feel free to edit or omit the second commit if you don't think it is useful.

Copy link
Member

@ObserverOfTime ObserverOfTime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to have a separate function for each predicate type (and one for the settings) and keep the if logic in the original function.

Comment on lines +152 to 155
for (long j = 0; j < steps; ++j) {
long nargs = 0;
for (; ; ++nargs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (long j = 0; j < steps; ++j) {
long nargs = 0;
for (; ; ++nargs) {
for (long j = 0, nargs; j < steps; ++j) {
for (nargs = 0; ; ++nargs) {

Assuming this works.

Copy link
Contributor Author

@Marcono1234 Marcono1234 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that works, but would that be really that useful? I don't think it helps with readability that much, and that enclosing for does not refer to nargs in the 'condition' or the 'step', so is there really any value in declaring the local variable in its 'init'?

Though if you prefer I can make that change nonetheless.

@Marcono1234
Copy link
Contributor Author

It might be better to have a separate function for each predicate type (and one for the settings) and keep the if logic in the original function.

Have tried to implement this now, I hope that is (roughly) what you had in mind. Let me know if you want something changed, or feel free to edit it yourself in case that is easier.

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