Deprecate regexp ok and error() methods #17245
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leekillough
requested review from
mstrout,
lydia-duncan,
ben-albrecht,
mppf and
bradcray
mppf
approved these changes
Feb 24, 2021
There was a problem hiding this comment.
Thanks for fixing it. I think it is reasonable to merge this PR in case anybody has old code that used these functions, and the modifications you made to re2-interface.cc seem reasonable. (I would also have found it OK to just make these functions call compilerError since they were not working).
I don't particularly care in this case (because it wasn't working before as you said) but note that the normal practice when deprecating is to include the deprecated functions in the module docs along with a note that they are deprecated.
ronawho
reviewed
Feb 24, 2021
Signed-off-by: Lee Killough <lee.killough@hpe.com>
2 tasks
e-kayrakli
added a commit
that referenced
this pull request
Mar 4, 2021
Close the leak coming from regexp.compile's error message QIO creates an error message when it fails to compile a regular expression. It returns a `const char *`. Then, we create a Chapel string from it. However, we aren't owning the error string buffer returned by QIO and that causes memory leaks. This PR makes the Chapel string own the buffer. This was a leak that we didn't know until @leekillough added deprecation test that hit that leak in #17245. But that test will disappear once we remove the functionality, so this PR adds a watered down version of the test to lock the behavior. [Reviewed by @mppf] #### Test status - [x] quickstart asan in `release/examples` and `regexp` - [x] standard
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This deprecates the
regexpokanderror()methods, since they were replaced with exceptions.This also fixes segfaults in
okanderror()when theregexpis not successfully compiled, or when it is default-initialized.The
error()method returns the same string as the compiler deprecation warning, since we do not have an easy way to access the original error message, because theregexpbeing returned fromcompile()is not assigned to a variable if an exception is thrown, so even if the exception is caught, the variable being assigned thecompile()result does not contain the error message -- only the exception does.This deserves a separate PR from #17189, since it is actually adding functionality, not removing it. It is not simply a reversion of PR #17189.