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

Fix regex callback #476

Closed
wants to merge 4 commits into from
Closed

Fix regex callback #476

wants to merge 4 commits into from

Conversation

sensetalkdoug
Copy link
Contributor

The Cocoa block uses a "stop" flag to allow a client to abort a long-running regex process. But the ICU library expects the callback function to return a flag with the opposite meaning -- "keepGoing". This change reverses the flag to fix the problem that any regex process that ran long enough to invoke the callback would be prematurely canceled.

The Cocoa callback block takes a pointer to a "stop" flag, but the ICU callback expects a return value with the opposite meaning (keepGoing). This error caused any longer-running regex to abort when it ran long enough to generate a callback instead of continuing as it should.
Add test to verify fix for callback which had logic reversed.
@sensetalkdoug sensetalkdoug requested a review from rfm as a code owner December 11, 2024 23:23
@johnathan-becker
Copy link

@rfm Can you please review this.

Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

While there's nothing actually wrong here, I think the new variable slightly detracts from the clarity of the implementation. Simply testing the value of 'stop' seems cleaner to me:

eg.

if (stop)
{
return FALSE;
}
else
{
return TRUE;
}

Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

I don't think the change to GNMUmakefile.preamble is correct (or necessary) as it will cause compilation failure on systems without blocks, and on systems with blocks the flag should be added automaticaly anyway.

On a related issue, the test itself needs to be conditionally compiled so that it doesn't run on systems without blocks or *(better) use the macros to simulate blocks if possible (see other blocks-specific testcases for an example).

@rfm
Copy link
Contributor

rfm commented Jan 4, 2025

I included parts of this in recent changes to render RE enumeration available to the classic compiler/runtime.

@rfm rfm closed this Jan 4, 2025
@sensetalkdoug
Copy link
Contributor Author

@rfm Sorry I didn't get back to you earlier. Thanks for implementing the fix (and test) in master.

@sensetalkdoug sensetalkdoug deleted the fix-regex-callback branch January 7, 2025 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants