Skip to content

MINOR: Use statically compiled regular expressions instead of inline#5042

Closed
KoenDG wants to merge 4 commits intoapache:trunkfrom
KoenDG:staticpattern2
Closed

MINOR: Use statically compiled regular expressions instead of inline#5042
KoenDG wants to merge 4 commits intoapache:trunkfrom
KoenDG:staticpattern2

Conversation

@KoenDG
Copy link
Copy Markdown
Contributor

@KoenDG KoenDG commented May 19, 2018

This should improve performance, as in-place means that the regex has to be compiled again each time the code path is traversed. Which is not performant.

A nice writeup on this topic can be found here: https://softwareengineering.stackexchange.com/questions/216320/java-regex-patterns-compile-time-constants-or-instance-members

And it's also recommended in Joshua Bloch's "Effective Java 3rd Edition" (Item 6: Avoid creating unnecessary objects)

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

…. This should improve performance, as in-place means that the regex has to be compiled again each time the code path is traversed. Which is not perfomant.
@KoenDG
Copy link
Copy Markdown
Contributor Author

KoenDG commented May 20, 2018

Pinging @guozhangwang

Can you assign someone to look at this? Sorry to bother you, just saw you assigning someone in this PR: #4745

Also, the builds seem to be gone so I can't check code coverage for these changes. Though the resulting functionality should be identical.

@guozhangwang guozhangwang requested a review from ijuma May 21, 2018 16:23
@guozhangwang
Copy link
Copy Markdown
Contributor

cc @ijuma for reviews.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 4, 2018

Thanks for the PR. The file Base64 has been removed so this needs to be rebased.

@KoenDG
Copy link
Copy Markdown
Contributor Author

KoenDG commented Jun 5, 2018

@ijuma Is it better if I rebase it, or just make a new PR on a new branch? So that it's 1 commit.

EDIT: Well, rebased it once I got home. Let me know if something else is needed.

EDIT2: Well hot damn, another conflict. I'll get to fixing it later.

KoenDG added 2 commits June 5, 2018 20:29
Base64 file removed, as in trunk.
Merge conflict fixed, adapted changes as per changed seen in trunk.
@KoenDG
Copy link
Copy Markdown
Contributor Author

KoenDG commented Jun 6, 2018

Third time's the charm, let's hope.

@KoenDG
Copy link
Copy Markdown
Contributor Author

KoenDG commented Jun 8, 2018

Closing to make a new one. Too many rebases and merges with trunk. I prefer there to be just 1 commit if at all possible.

@KoenDG KoenDG closed this Jun 8, 2018
@KoenDG KoenDG deleted the staticpattern2 branch June 8, 2018 20:21
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.

3 participants