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

Remove method with definite value for simplicity #5409

Closed
wants to merge 0 commits into from
Closed

Remove method with definite value for simplicity #5409

wants to merge 0 commits into from

Conversation

jamesgua
Copy link
Contributor

those methods return false and not referenced in other places except local
remove unused variable in member function

Signed-off-by: James Guan jamesgua@ca.ibm.com

@@ -475,10 +475,8 @@ bool SimpleRegex::match(
bool isCaseSensitive)
{
TR::Compilation *comp = TR::comp();
TR::StackMemoryRegion stackMemoryRegion(*comp->trMemory());
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the memory lifetime of the data returned from signature and, possibly regex - why the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize the destructor will unregister the region, I was thinking it's unused instance, will revert the change and update.

@andrewcraik andrewcraik self-assigned this Jul 22, 2020
@andrewcraik
Copy link
Contributor

Was the last commit supposed to address the review? I don't see the region being restored when I am looking at the code on github...

@jamesgua
Copy link
Contributor Author

Struggling with messed commits, still learning to merge them properly.

@jamesgua
Copy link
Contributor Author

@andrewcraik Now pls take a look of updated PR.

@andrewcraik
Copy link
Contributor

@genie-omr build all

@andrewcraik
Copy link
Contributor

@jamesgua I see you have added an extra commit with changes unrelated to the PR - can you please remove them and open a new PR for those if you wish to contribute them? We try to keep each PR to a logical change and these two changes appear to be unrelated.

@andrewcraik
Copy link
Contributor

@jamesgua have you had a chance to look at this? The contribution you were looking to make would be welcome - if you could remove the unrelated change we can look to retest and accept the contribution - thanks!

@jamesgua jamesgua closed this Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants