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 date regexp #447

Merged
merged 4 commits into from
Oct 11, 2020
Merged

Remove date regexp #447

merged 4 commits into from
Oct 11, 2020

Conversation

pelletier
Copy link
Owner

@pelletier pelletier commented Oct 11, 2020

Issue: #440

Hand-roll the date matching logic to avoid trying to match a regexp on
every integer.

benchmark                    old ns/op     new ns/op     delta
BenchmarkUnmarshalToml-8     293449        272134        -7.26%

benchmark                    old allocs     new allocs     delta
BenchmarkUnmarshalToml-8     2746           2650           -3.50%

benchmark                    old bytes     new bytes     delta
BenchmarkUnmarshalToml-8     133604        127548        -4.53%

A lot of the new code looks duplicated. It's not quite. I found it easier to read and more precise in the reported errors than the various refactors I tried.

@moorereason

Hand-roll the date matching logic to avoid trying to match a regexp on
every integer.

```
benchmark                    old ns/op     new ns/op     delta
BenchmarkUnmarshalToml-8     293449        272134        -7.26%

benchmark                    old allocs     new allocs     delta
BenchmarkUnmarshalToml-8     2746           2650           -3.50%

benchmark                    old bytes     new bytes     delta
BenchmarkUnmarshalToml-8     133604        127548        -4.53%
```
@codecov
Copy link

codecov bot commented Oct 11, 2020

Codecov Report

Merging #447 into master will increase coverage by 0.23%.
The diff coverage is 98.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
+ Coverage   93.32%   93.56%   +0.23%     
==========================================
  Files          10       10              
  Lines        2158     2253      +95     
==========================================
+ Hits         2014     2108      +94     
- Misses        104      105       +1     
  Partials       40       40              
Impacted Files Coverage Δ
token.go 100.00% <ø> (ø)
parser.go 90.33% <91.30%> (-0.34%) ⬇️
lexer.go 98.91% <100.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9ba082...50e0035. Read the comment docs.

@moorereason
Copy link
Contributor

LGTM 👍

The company has been acquired by GitLab and shutting down.
@pelletier pelletier merged commit 5c66c78 into master Oct 11, 2020
@pelletier pelletier deleted the regexps branch October 11, 2020 19:31
@pelletier pelletier added the performance Issue related to a performance problem or pull request improving performance. label Oct 11, 2020
@pelletier pelletier linked an issue Oct 11, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue related to a performance problem or pull request improving performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor date detection in lexer
2 participants