Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1311)

Issue 14162044: code review 14162044: go.text/language: revamped error handling: (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by mpvl
Modified:
11 years, 4 months ago
Reviewers:
r
CC:
r, mpvl_google.com, golang-dev
Visibility:
Public.

Description

go.text/language: revamped error handling: - ValueError now exported as new type. ValueError retains the problematic value, allowing the user to inspect and correct it. - Dynamically allocated errors returned in case of a syntax error are replaced by a error variable. - Fixed bug: return error if an "u" extension has a type without a value. - Added benchmarks or parsing code. - Renamed MissingLikelyData to ErrMissingLikelyData to be consistent with other Go packages. This variable is not yet returned, so this change is not likely to cause a big issue. - Removed Set type as long as there is no demand for it. The code is measurably faster after removing the dynamically allocated errors. A ValueError is 8 bytes and should not require allocation when passed as an error. Returning a fixed error variable instead of a ValueError did not significantly improve performance. I considered returning a syntax error with the position at which the error occurred. This extra management needed for this slowed down the code a bit, so I opted not to support this. This could still be implemented if there turns out to be a need for it.

Patch Set 1 #

Patch Set 2 : diff -r 565aa3af64cd https://code.google.com/p/go.text #

Patch Set 3 : diff -r 565aa3af64cd https://code.google.com/p/go.text #

Total comments: 1

Patch Set 4 : diff -r 565aa3af64cd https://code.google.com/p/go.text #

Patch Set 5 : diff -r 565aa3af64cd https://code.google.com/p/go.text #

Patch Set 6 : diff -r 565aa3af64cd https://code.google.com/p/go.text #

Total comments: 4

Patch Set 7 : diff -r d1f38b6467f7 https://code.google.com/p/go.text #

Unified diffs Side-by-side diffs Delta from patch set Stats (+286 lines, -95 lines) Patch
M language/examples_test.go View 1 2 3 5 chunks +36 lines, -5 lines 0 comments Download
M language/language.go View 1 2 3 4 5 6 5 chunks +12 lines, -22 lines 0 comments Download
M language/language_test.go View 1 1 chunk +101 lines, -0 lines 0 comments Download
M language/lookup.go View 1 2 3 7 chunks +21 lines, -19 lines 0 comments Download
M language/lookup_test.go View 1 2 3 1 chunk +5 lines, -5 lines 0 comments Download
M language/match.go View 1 2 3 4 5 6 2 chunks +4 lines, -4 lines 0 comments Download
M language/parse.go View 1 2 3 4 5 6 18 chunks +75 lines, -38 lines 0 comments Download
M language/parse_test.go View 1 2 3 3 chunks +32 lines, -2 lines 0 comments Download

Messages

Total messages: 7
mpvl
Hello r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.text
11 years, 4 months ago (2013-10-04 18:31:56 UTC) #1
r
https://codereview.appspot.com/14162044/diff/6001/language/lookup.go File language/lookup.go (right): https://codereview.appspot.com/14162044/diff/6001/language/lookup.go#newcode125 language/lookup.go:125: return 0, SyntaxError{} why not predefine an unexported instance ...
11 years, 4 months ago (2013-10-04 18:35:56 UTC) #2
mpvl_google.com
Mostly to have symmetry with ValueError and that users know they will get one or ...
11 years, 4 months ago (2013-10-04 19:01:12 UTC) #3
mpvl
I've changed to use an internal variable instead of a type and uploaded the changes. ...
11 years, 4 months ago (2013-10-06 07:16:33 UTC) #4
r
LGTM https://codereview.appspot.com/14162044/diff/34001/language/language.go File language/language.go (right): https://codereview.appspot.com/14162044/diff/34001/language/language.go#newcode347 language/language.go:347: // It returns a ValueError if s is ...
11 years, 4 months ago (2013-10-08 16:27:22 UTC) #5
mpvl
https://codereview.appspot.com/14162044/diff/34001/language/language.go File language/language.go (right): https://codereview.appspot.com/14162044/diff/34001/language/language.go#newcode347 language/language.go:347: // It returns a ValueError if s is a ...
11 years, 4 months ago (2013-10-08 17:45:24 UTC) #6
mpvl
11 years, 4 months ago (2013-10-08 17:51:07 UTC) #7
*** Submitted as
https://code.google.com/p/go/source/detail?r=4a0a2a704c87&repo=text ***

go.text/language: revamped error handling:
- ValueError now exported as new type. ValueError retains the problematic value,
  allowing the user to inspect and correct it.
- Dynamically allocated errors returned in case of a syntax error are replaced
  by a error variable.
- Fixed bug: return error if an "u" extension has a type without a value.
- Added benchmarks or parsing code.
- Renamed MissingLikelyData to ErrMissingLikelyData to be consistent with other
  Go packages. This variable is not yet returned, so this change is not likely
to cause
  a big issue.
- Removed Set type as long as there is no demand for it.

The code is measurably faster after removing the dynamically allocated errors.
A ValueError is 8 bytes and should not require allocation when passed as an
error.
Returning a fixed error variable instead of a ValueError did not significantly
improve
performance.

I considered returning a syntax error with the position at which the error
occurred.
This extra management needed for this slowed down the code a bit, so I opted not
to
support this. This could still be implemented if there turns out to be a need
for it.

R=r, mpvl
CC=golang-dev
https://codereview.appspot.com/14162044
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b