Issue 151580043: code review 151580043: go.test/runes: added package for manipulating runes in text.

Can't Edit
Can't Publish+Mail
Start Review

Created:
9 years, 6 months ago by mpvl

Modified:
9 years, 4 months ago

Reviewers:
nigeltao

CC:
nigeltao, r, golang-codereviews

Visibility:
Public.

Description

go.text/runes: added package for manipulating runes in text. Mostly for defining transformer equivalents for functionality found in core's strings and bytes packages. Design points: - Functionilty is now a bit scattered (e.g. in encoding and transform). - RemoveFunc in transform is Remove in this package. The func was a bit misplaced in transform. - Adding these to transform would clutter it too much. Also, the semantics of runes.Map is clearer from the syntax than transform.Map. - Did not use unicode, strings or bytes as package name to avoid conflicts: it is not unlikely these packages will be used together in one file. - Naming of the funcs returning Transformers describe the action performed by the transformer, not the type of Transfomer they return. This is maybe a bit odd, but when adopted as a convention within go.text, it reads quite nicely (compare with cases.Upper, for example). - Used a wrapper around Transformer similar to Caser (Transform migh be a better name). I expect there to be enough use cases for this to warrant it and since all the transforms wrapped in this type don't return errors, the resulting API is much simpler: t := runes.Map(myFunc) s, _, _ := transform.String(t, s) fmt.Println(s) versus fmt.Println(runes.Map(myFunc).String(s)) (runes.Map(myFunc) is fast and does not cause an allocation.)

Patch Set 1 #

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

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

Total comments: 24

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

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

Total comments: 23

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

Total comments: 53

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

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

Total comments: 6

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

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

Created: 9 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+870 lines, -0 lines)			Patch
A	runes/example_test.go	View	1 chunk	+46 lines, -0 lines	0 comments	Download
A	runes/runes.go	View	1 chunk	+254 lines, -0 lines	0 comments	Download
A	runes/runes_test.go	View	1 chunk	+570 lines, -0 lines	0 comments	Download

Messages

Total messages: 16

Expand All Messages | Collapse All Messages

mpvl

Hello nigeltao@golang.org (cc: golang-codereviews@googlegroups.com, r@golang.org), I'd like you to review this change to https://code.google.com/p/go.text

9 years, 6 months ago (2014-10-08 14:00:02 UTC) #1

the repo is go.text not go.test. please fix CL subject and remail

9 years, 6 months ago (2014-10-08 18:04:41 UTC) #2

https://codereview.appspot.com/151580043/diff/20002/runes/runes.go File runes/runes.go (right): https://codereview.appspot.com/151580043/diff/20002/runes/runes.go#newcode21 runes/runes.go:21: // Validator is a transformer that returns ErrInvalid on ...

9 years, 6 months ago (2014-10-08 18:10:44 UTC) #3

nigeltao

https://codereview.appspot.com/151580043/diff/20002/runes/example_test.go File runes/example_test.go (right): https://codereview.appspot.com/151580043/diff/20002/runes/example_test.go#newcode28 runes/example_test.go:28: replaceHyhpens := runes.Map(func(r rune) rune { Typo in "Hyphens". ...

9 years, 6 months ago (2014-10-09 06:03:59 UTC) #4

mpvl

9 years, 6 months ago (2014-10-09 07:45:33 UTC) #5

mpvl

(addressed your comments in the replay to r). On Thu, Oct 9, 2014 at 8:03 ...

9 years, 6 months ago (2014-10-09 07:46:42 UTC) #6

nigeltao

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go File runes/runes.go (right): https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode24 runes/runes.go:24: ValidateWellFormed transform.Transformer = nil // TODO: Validate(nil) If Validate ...

9 years, 6 months ago (2014-10-21 02:45:46 UTC) #7

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go
File runes/runes.go (right):

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode24
runes/runes.go:24: ValidateWellFormed transform.Transformer = nil // TODO:
Validate(nil)
If Validate isn't implemented yet, then leave it (and ValidateWellFormed and
ErrInvalid) out of this CL and add it in a future CL??

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode32
runes/runes.go:32: RuneError = utf8.RuneError
Do you expect users of this package to refer to this constant? If not, don't
export it: s/RuneError/runeError/

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode64
runes/runes.go:64: b, _, _ = transform.Bytes(t, b)
Huh, if you're always dropping the other return values, then maybe
transform.Bytes should just return []byte, and you wouldn't need this
runes.Transformer type. You could then also take out the Bytes and String
methods, and possibly the Caser type, of
https://codereview.appspot.com/122420043/diff/220001/cases/cases.go

Maybe we should make that change to the transform package.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode68
runes/runes.go:68: // Bytes returns a string holding the result of transforming
s using the
s/Bytes/String/

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode86
runes/runes.go:86: func ReplaceIllFormedWithRune(r rune) Transformer {
Would we ever want anything other than r == RuneError?

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode94
runes/runes.go:94: // The benchmark for this special-cased mapping indicates
about a -75% running
The performance comment doesn't have to be a doc comment. Put it inside the
struct, perhaps?

A general CL management comment is that I don't think the initial check-in has
to be super-efficient. As long as we get the API right (and concious of things
like garbage), we can check in the simple and obviously correct implementation
first, and check in the optimized implementation in a follow-up. Benefits of
optimizing in a follow-up CL include (1) you're optimizing from a point where
the tests are know to be good, as opposed to checking in new tests at the same
time as new (optimized) code, and (2) people other than the original author can
reproduce any before-optimization vs after-optimization benchmarks.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode98
runes/runes.go:98: type wellFormed struct {
Rename wellFormed to replaceIllFormed?

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode110
runes/runes.go:110: if len(dst) < len(src) {
As an example of my general point about optimizations: is this fragment of code
here just an optimization or is it important for performance? It's not
immediately obvious.

As an alternative, you could make the initial check-in the simpler:

func (t wellFormed) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err
error) {
  for nSrc < len(src) {
    r, size := utf8.DecodeRune(src[nSrc:])

    // Look for an ASCII rune.
    if r < utf8.RuneSelf {
      if nDst == len(dst) {
        err = transform.ErrShortDst
        break
      }
      dst[nDst] = byte(r)
      nDst++
      nSrc++
      continue
    }

    // Look for a valid non-ASCII rune.
    if r != RuneError || size != 1 {
      if size != copy(dst[nDst:], src[nSrc:nSrc+size]) {
        err = transform.ErrShortDst
        break
      }
      nDst += size
      nSrc += size
      continue
    }

    // Look for short source data.
    if !atEOF && !utf8.FullRune(src[nSrc:]) {
      err = transform.ErrShortSrc
      break
    }

    // We have an invalid rune.
    if int(t.size) != copy(dst[nDst:], t.replacement[:t.size]) {
      err = transform.ErrShortDst
      break
    }
    nDst += int(t.size)
    nSrc++
  }
  return nDst, nSrc, err
}

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode117
runes/runes.go:117: nSrc++
I'd add a continue after this, and also a few lines down, and then outdent the
larger "else" block.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode122
runes/runes.go:122: pDst += copy(dst[pDst:], src[pSrc:nSrc])
It's not obvious whether or not it's problematic if the copy doesn't finish on a
rune boundary (but you still increment pDst). Is it always OK, because of the
"if len(dst) < len(src)" above?? If you're relying on subtle invariants like
this, I'd like some more commentary.

https://codereview.appspot.com/151580043/diff/70001/runes/runes_test.go
File runes/runes_test.go (right):

https://codereview.appspot.com/151580043/diff/70001/runes/runes_test.go#newco...
runes/runes_test.go:553: dst := make([]byte,
Weird line break.

mpvl

9 years, 6 months ago (2014-10-21 16:43:44 UTC) #8

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go
File runes/runes.go (right):

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode24
runes/runes.go:24: ValidateWellFormed transform.Transformer = nil // TODO:
Validate(nil)
On 2014/10/21 02:45:45, nigeltao wrote:
> If Validate isn't implemented yet, then leave it (and ValidateWellFormed and
> ErrInvalid) out of this CL and add it in a future CL??

Sure.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode32
runes/runes.go:32: RuneError = utf8.RuneError
On 2014/10/21 02:45:45, nigeltao wrote:
> Do you expect users of this package to refer to this constant? If not, don't
> export it: s/RuneError/runeError/

Yes, I think it is a useful constant for this package, and it saves the user
from having to import the utf8 package.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode64
runes/runes.go:64: b, _, _ = transform.Bytes(t, b)
On 2014/10/21 02:45:45, nigeltao wrote:
> Huh, if you're always dropping the other return values, then maybe
> transform.Bytes should just return []byte, and you wouldn't need this
> runes.Transformer type.
That is what I had originally. There are some transforms, however, where the
result indeed may be an error. For the transforms in cases and runes, the
transforms never return an error (other than ErrShort*), so it is safe to ignore
the return values.

Arguably, transform.Bytes and transform.String can indeed be defined to only be
valid for transforms that don't return exceptional errors.  That said, for
things as common as cases.Lower().String(...), for example, it would be a bit
annoying to have to write transform.String(Cases().Lower(), ...), imo.

> You could then also take out the Bytes and String
> methods, and possibly the Caser type, of
> https://codereview.appspot.com/122420043/diff/220001/cases/cases.go
> 
> Maybe we should make that change to the transform package.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode68
runes/runes.go:68: // Bytes returns a string holding the result of transforming
s using the
On 2014/10/21 02:45:45, nigeltao wrote:
> s/Bytes/String/

Done.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode86
runes/runes.go:86: func ReplaceIllFormedWithRune(r rune) Transformer {
On 2014/10/21 02:45:45, nigeltao wrote:
> Would we ever want anything other than r == RuneError?

I'm not sure if it is wise, but it seems a common thing people want to do.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode94
runes/runes.go:94: // The benchmark for this special-cased mapping indicates
about a -75% running
On 2014/10/21 02:45:45, nigeltao wrote:
> The performance comment doesn't have to be a doc comment. Put it inside the
> struct, perhaps?
Done. Although isn't fine to leave at the struct level as it is an unexported
type?

> A general CL management comment is that I don't think the initial check-in has
> to be super-efficient. As long as we get the API right (and concious of things
> like garbage), we can check in the simple and obviously correct implementation
> first, and check in the optimized implementation in a follow-up. Benefits of
> optimizing in a follow-up CL include (1) you're optimizing from a point where
> the tests are know to be good, as opposed to checking in new tests at the same
> time as new (optimized) code, and (2) people other than the original author
can
> reproduce any before-optimization vs after-optimization benchmarks.
Noted.  Sometimes an API design may depend on the feasibility of making the
operation efficient, though. In this case I wanted to know if it pays off to
have special version for replacing illformed runes only, vs just supporting map.

You bring up good points, though. Note that in this case, you can still compare
the before and afters, as it is compared against Map.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode98
runes/runes.go:98: type wellFormed struct {
On 2014/10/21 02:45:45, nigeltao wrote:
> Rename wellFormed to replaceIllFormed?

Done.

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode110
runes/runes.go:110: if len(dst) < len(src) {
On 2014/10/21 02:45:45, nigeltao wrote:
> As an example of my general point about optimizations: is this fragment of
code
> here just an optimization or is it important for performance? It's not
> immediately obvious.
Point taken. Will check in the simpler version. 

As an example of my point, though: The running time of BenchmarkWellFormed for
the simpler version is about +400% relative to the optimized version. At this
running time, it has roughly the same running time as Map. Including
ReplaceIllFormed would have some benefits of a slightly better API for this
common case and allowing illegal bytes to be replaced with something other than
U+FFFD, but it would have been worth considering not including it in the API. I
suspected it could be made much more efficient than Map, though. By doing the
optimization and seeing the results it was clear it is useful to special-case
this common case.

> 
> As an alternative, you could make the initial check-in the simpler:
> 
> func (t wellFormed) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int,
err
> error) {
>   for nSrc < len(src) {
>     r, size := utf8.DecodeRune(src[nSrc:])
> 
>     // Look for an ASCII rune.
>     if r < utf8.RuneSelf {
>       if nDst == len(dst) {
>         err = transform.ErrShortDst
>         break
>       }
>       dst[nDst] = byte(r)
>       nDst++
>       nSrc++
>       continue
>     }
> 
>     // Look for a valid non-ASCII rune.
>     if r != RuneError || size != 1 {
>       if size != copy(dst[nDst:], src[nSrc:nSrc+size]) {
>         err = transform.ErrShortDst
>         break
>       }
>       nDst += size
>       nSrc += size
>       continue
>     }
> 
>     // Look for short source data.
>     if !atEOF && !utf8.FullRune(src[nSrc:]) {
>       err = transform.ErrShortSrc
>       break
>     }
> 
>     // We have an invalid rune.
>     if int(t.size) != copy(dst[nDst:], t.replacement[:t.size]) {
>       err = transform.ErrShortDst
>       break
>     }
>     nDst += int(t.size)
>     nSrc++
>   }
>   return nDst, nSrc, err
> }

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode117
runes/runes.go:117: nSrc++
On 2014/10/21 02:45:45, nigeltao wrote:
> I'd add a continue after this, and also a few lines down, and then outdent the
> larger "else" block.

Acknowledged. (Done in saved version.)

https://codereview.appspot.com/151580043/diff/70001/runes/runes.go#newcode122
runes/runes.go:122: pDst += copy(dst[pDst:], src[pSrc:nSrc])
On 2014/10/21 02:45:45, nigeltao wrote:
> It's not obvious whether or not it's problematic if the copy doesn't finish on
a
> rune boundary (but you still increment pDst). Is it always OK, because of the
> "if len(dst) < len(src)" above?? If you're relying on subtle invariants like
> this, I'd like some more commentary.

Correct. They copy copies bytes that have already been verified to be correct
and fit in dst (the copy will always finish on a rune boundary).

Will add comment in saved version.

nigeltao

Despite my comments, the code is pretty close to Looking Good. I'm not entirely convinced ...

9 years, 6 months ago (2014-10-22 05:37:45 UTC) #9

mpvl

The argument for the package name "runes" is as follows: - All operations operate on ...

9 years, 6 months ago (2014-10-22 11:22:38 UTC) #10

The argument for the package name "runes" is as follows:
- All operations operate on one codepoint at a time.
- unicode could be appropriate, but would often results in conflicts, as
RangeTables are used together with this package.
- Even though the operations are not defined on []rune, they are defined on
sequences of rune, so the name is not too far off and somewhat in line with
strings and bytes.
- The semantics transform.Map and transform.Remove is not clear from the name.
(Map and Remove could also operate on arbitrary strings, grapheme clusters, or
whatever.)

Let's see what r has to say.

https://codereview.appspot.com/151580043/diff/70001/runes/runes_test.go
File runes/runes_test.go (right):

https://codereview.appspot.com/151580043/diff/70001/runes/runes_test.go#newco...
runes/runes_test.go:553: dst := make([]byte,
On 2014/10/22 05:37:43, nigeltao wrote:
> On 2014/10/21 02:45:45, nigeltao wrote:
> > Weird line break.
> 
> Ping.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/example_test.go
File runes/example_test.go (right):

https://codereview.appspot.com/151580043/diff/90001/runes/example_test.go#new...
runes/example_test.go:41: rm := runes.Remove(runes.InString("aeiou"))
Please consider 151590043 and 151660044. Together they implement the equivalent
for InString and NotInString for RangeTables.

I'm not averse to removing these functions, but please consider these CLs.

On 2014/10/22 05:37:43, nigeltao wrote:
> I don't have a lot of Unicode experience to bear, but I'm not sure if the
> InString helper is worth it. Unlike C++ or Java, it's still pretty easy for
> programmers to say
> 
> rm := runes.Remove(func (r rune) bool{ return strings.Contains("aeiou", r) })
> 
> a la your isModifier example above. Also, it's not like you're providing
> runes.InRange, runes.NotInRange, runes.Conjunction, runes.Disjunction or
> runes.Not helper functions. We have to draw a line somewhere, and it seems
easy
> to draw it at no helpers at all.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go
File runes/runes.go (right):

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode27
runes/runes.go:27: type InFunc func(r rune) bool
> second thoughts, I just wouldn't
> name the type: "func(rune) bool" describes exactly what it is, and "func
> InString(s string) func(rune) bool" is perfectly readable, a la
> http://blog.golang.org/gos-declaration-syntax

The main reason for this type is that the functionality like InString etc. gets
nicely collated in the Docs. If we dump these, I agree this type should go. If
we don't, I'm open to removing the type anyway, although the type does make the
doc a lot cleaner, imo.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode27
runes/runes.go:27: type InFunc func(r rune) bool
On 2014/10/22 05:37:44, nigeltao wrote:
> I still prefer Predicate over InFunc, but on 
I like it as well, but as you suggested, was waiting for r's feedback.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode34
runes/runes.go:34: 
On 2014/10/22 05:37:44, nigeltao wrote:
> Delete the blank line.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode68
runes/runes.go:68: ReplaceIllFormed Transformer =
ReplaceIllFormedWithRune(RuneError)
On 2014/10/22 05:37:43, nigeltao wrote:
> I'd delete this, and below, rename func ReplaceIllFormedWithRune to
> ReplaceIllFormed
The reason for this design is to encourage people to use U+FFFD by default. It
should be easier to use this default than to do anything else. Your suggested
change makes just as easy to replace it with something else.

One alternative is to have an Option pattern a la:
  ReplaceIllFormed(o ...Option)

func Replacement(r rune) Option

type Option func(*options)

I considered this before. I thought it was a bit overkill, but it may be the
nicest solution, actually. The option could apply to Map and Remove as well.

[other consideration: Using this approach, one could consider dropping
ReplaceIllFormed and using Map or Remove instead. For example,
ReplaceIllFormed() would become Map(nil), and ReplaceIllFormed(Replacement('?'))
would become Map(nil, Replacement('?')).  I considered this, but decided against
this for a few reasons:
   - Performance of Map(nil) and Map(func(r rune) rune { return r }) would be
different.
   - A bit not obvious to allow a nil function.
   - To be consistent, Remove(nil) should be defined as well, and would be the
same as Map(nil), which is weird.
 As ReplaceIllFormed is very common, I figured that explicitly defining it and
not allowing nil functions for Map and Remove is better.]

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode75
runes/runes.go:75: func ReplaceIllFormedWithRune(r rune) Transformer {
On 2014/10/22 05:37:44, nigeltao wrote:
> BTW, probably for a future CL, but would we ever want to replace ill-formed
> bytes with empty strings instead of a specific rune?

I had this in my first implementation. Replacing with a string can be useful
(e.g. replacing it with some HTML tag). However, removing illegal bytes is
considered to be bad form, so I didn't want to make that too easy. Maybe the way
to go is to add the functionality later with a clear comment that removing
illegal bytes is bad form. Using the Option scheme described, it could be
introduced as 
func ReplacementString(s string) Option

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode87
runes/runes.go:87: // about -50% for the input string used by the benchmarks.
On 2014/10/22 05:37:44, nigeltao wrote:
> I'd add a blank line after this, since this comment isn't specifically about
the
> transform.NopResetter embedded field.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode140
runes/runes.go:140: func Remove(f InFunc) Transformer {
On 2014/10/22 05:37:44, nigeltao wrote:
> Also remove transform.RemoveFunc, in ../transform/transform.go, in this CL?

I was planning to do this in a followup CL.  I think it is good to give people a
chance to remove it without breaking a build or something else they care about,
which means both must exist for a brief while at least:
- update go.text to a version including this package.
- remove usage of transform.RemoveFunc
- it is not safe to update to a version of go.text not including RemoveFunc

Added a TODO.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode151
runes/runes.go:151: 
On 2014/10/22 05:37:44, nigeltao wrote:
> Delete the blank line.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode182
runes/runes.go:182: if !t(r) {
On 2014/10/22 05:37:44, nigeltao wrote:
> I'd say:
> 
> if t(r) {
>   nSrc += size
>   continue
> }
> 
> and out-dent the rest.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode215
runes/runes.go:215: 
On 2014/10/22 05:37:44, nigeltao wrote:
> Delete the blank line.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode256
runes/runes.go:256: var b [utf8.UTFMax]byte
On 2014/10/22 05:37:44, nigeltao wrote:
> Hoist this out of the loop, to avoid zero-ing it on every iteration.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go
File runes/runes_test.go (right):

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:21: in, out, outFull string
On 2014/10/22 05:37:44, nigeltao wrote:
> Personally, since the struct value fields are one-per-line, I'd split this up
> here:
> 
> in string
> out string
> outFull string

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:132: repl:    "0",
On 2014/10/22 05:37:44, nigeltao wrote:
> Maybe s/0/?/ would be closer to what people do with real programs?

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:168: wf.size = uint8(utf8.EncodeRune(wf.replacement[:],
[]rune(tt.repl)[0]))
On 2014/10/22 05:37:44, nigeltao wrote:
> wf.size = uint8(copy(wf.replacement[:], tt.repl))
> 
> and then you don't need the `if tt.repl != ""` check.
> 
> Or change tt.repl's type from string to rune and make this:
> wf := ReplaceIllFormed(tt.repl)
> where tt.repl < 0 means 'replace with the empty string'.

Doing first suggestion, in case we'll allow for string replacement at some
point.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:173: if nDst != len(tt.out) {
On 2014/10/22 05:37:44, nigeltao wrote:
> This check seems redundant with checking
> if got := string(dst[:nDst]); got != tt.out {
> a few lines down.

Removed

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:183: // We perform this part of the test mostly to test
nSrc.
Updated the comment.

On 2014/10/22 05:37:44, nigeltao wrote:
> It's not immediately obvious what the next bit has to do with nSrc.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:185: if want == "" {
On 2014/10/22 05:37:44, nigeltao wrote:
> I'd prefer explicitly duplicating the out/outFull fields in the values above
> instead of implicitly having empty outFull means that outFull = out".

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:199: in, out, outFull string
On 2014/10/22 05:37:44, nigeltao wrote:
> Ditto.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:341: if nDst != len(tt.out) {
On 2014/10/22 05:37:45, nigeltao wrote:
> Ditto, or factor out a
> func check(t *testing.T, szDst int, in string, atEOF bool, want, wantFull
> string, wantErr error)
> function and use it for TestWellFormed, TestRemove and TestMap.

Factored test out.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:378: in, out, outFull string
On 2014/10/22 05:37:45, nigeltao wrote:
> Ditto.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:510: if nDst != len(tt.out) {
On 2014/10/22 05:37:44, nigeltao wrote:
> Ditto.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:551: func BenchmarkWellFormed(t *testing.B) {
On 2014/10/22 05:37:44, nigeltao wrote:
> s/t/b/ is traditional. Similarly below.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:552: txt := input
On 2014/10/22 05:37:44, nigeltao wrote:
> Just inline "input" into the next two lines.
> 
> Similarly below.

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:558: wf.size = uint8(copy(wf.replacement[:],
runeErrorString))
On 2014/10/22 05:37:44, nigeltao wrote:
> Alternatively,
> wr := ReplaceIllFormed(RuneError)

Done.

https://codereview.appspot.com/151580043/diff/90001/runes/runes_test.go#newco...
runes/runes_test.go:558: wf.size = uint8(copy(wf.replacement[:],
runeErrorString))
On 2014/10/22 05:37:44, nigeltao wrote:
> Alternatively,
> wr := ReplaceIllFormed(RuneError)

Done.

mpvl

https://codereview.appspot.com/151580043/diff/90001/runes/runes.go File runes/runes.go (right): https://codereview.appspot.com/151580043/diff/90001/runes/runes.go#newcode68 runes/runes.go:68: ReplaceIllFormed Transformer = ReplaceIllFormedWithRune(RuneError) In fact, I've renamed the ...

9 years, 6 months ago (2014-10-22 13:21:39 UTC) #11

i'm not convinced runes is the right name either, since by analogy with bytes we'd ...

9 years, 6 months ago (2014-10-22 15:16:01 UTC) #12

nigeltao

LGTM, but wait for r. I'd still cut the API surface down, as it's easier ...

9 years, 6 months ago (2014-10-23 03:48:09 UTC) #13

Marcel: Please mail the godoc output for this package. I find that's easier to absorb ...

9 years, 6 months ago (2014-10-23 03:53:09 UTC) #14

Marcel: Please mail the godoc output for this package. I find that's easier
to absorb than the code version (and I don't have to patch in your CL).

-rob


On Wed, Oct 22, 2014 at 8:48 PM, <nigeltao@golang.org> wrote:

> LGTM, but wait for r.
>
> I'd still cut the API surface down, as it's easier to grow an API later
> than to shrink one, but if both you and r are happy then I'll concede.
>
> As for package names, I still prefer transform.RemoveRunes and
> transform.MapRunes over runes.Remove and runes.Map, but again, I'm happy
> to concede the argument.
>
>
> https://codereview.appspot.com/151580043/diff/90001/runes/example_test.go
> File runes/example_test.go (right):
>
> https://codereview.appspot.com/151580043/diff/90001/runes/example_test.go#
> newcode41
> runes/example_test.go:41: rm := runes.Remove(runes.InString("aeiou"))
> On 2014/10/22 11:22:37, mpvl wrote:
>
>> Please consider 151590043 and 151660044. Together they implement the
>>
> equivalent
>
>> for InString and NotInString for RangeTables.
>>
>
>  I'm not averse to removing these functions, but please consider these
>>
> CLs.
>
> Even with a RangeTables.In method, I'd err on the side of leaving these
> InString, NotInString and InFunc definitions out until there's a clear
> need for it.
>
> https://codereview.appspot.com/151580043/diff/130001/runes/example_test.go
> File runes/example_test.go (right):
>
> https://codereview.appspot.com/151580043/diff/130001/
> runes/example_test.go#newcode34
> runes/example_test.go:34:
> fmt.Println(replaceHyphens.String("a-b‐c⸗d﹣e"))
> I can see the motivation for the runes.Remove function, which returns
> something to put in a transform.Chain, but I'm not convinced that this
> example is a compelling justification for the Transformer.String method
> that you define in this CL. This snippet could just as easily be a call
> to strings.Map from the standard library.
>
> https://codereview.appspot.com/151580043/diff/130001/runes/runes.go
> File runes/runes.go (right):
>
> https://codereview.appspot.com/151580043/diff/130001/
> runes/runes.go#newcode45
> runes/runes.go:45: type Transformer struct {
> I'm not convinced that this Transformer type (and its Bytes and Strings
> methods) are necessary, but I'm not going to insist. (See my comment in
> example_test.go).
>
> https://codereview.appspot.com/151580043/diff/130001/
> runes/runes.go#newcode66
> runes/runes.go:66: return Transformer{&replaceIllFormed{}}
> You could drop the &.
>
> https://codereview.appspot.com/151580043/diff/130001/runes/runes_test.go
> File runes/runes_test.go (right):
>
> https://codereview.appspot.com/151580043/diff/130001/
> runes/runes_test.go#newcode146
> runes/runes_test.go:146: 16: {
> s/16/11/
>
> https://codereview.appspot.com/151580043/
>

mpvl

See below. It may be useful to consider this package together with the two CLs ...

9 years, 6 months ago (2014-10-23 07:36:37 UTC) #15

See below.

It may be useful to consider this package together with the two CLs for
unicode. I've included the godoc diffs for this as well.

======= GoDoc for runes:

package runes
    import "."

    Package runes contains functionality for manipulating runes in text.

CONSTANTS

const (
    // RuneError is the default replacement rune for replacing invalid
Unicode.
    RuneError = utf8.RuneError
)

TYPES

type InFunc func(r rune) bool
    A InFunc defines a set of runes.

func InString(s string) InFunc
    InString returns an InFunc that selects all the runes contained in s.

func NotInString(s string) InFunc
    NotInString returns an InFunc that excludes all the runes contained in
    s.

type Transformer struct {
    transform.Transformer
}
    A Transformer encapsulates and adds functionality to the Transformer
    interface defined in the transform package.

func Map(mapping func(rune) rune) Transformer
    Map returns a Transformer that maps the runes in the input using the
    given mapping. Illegal bytes in the input are converted to RuneError
    before being passed to the mapping func.

func Remove(f InFunc) Transformer
    Remove returns a transformer that removes runes r for which f(r) is
    true. Illegal input bytes are replaced by RuneError before being passed
    to f.

func ReplaceIllFormed() Transformer
    ReplaceIllFormed returns a transformer that replaces all input bytes
    that are not part of a well-formed UTF-8 code sequence with RuneError.

func (t Transformer) Bytes(b []byte) []byte
    Bytes returns a new byte slice holding the result of transforming b
    using the transform.

func (t Transformer) String(s string) string
    String returns a string holding the result of transforming s using the
    transform.


====== Related GoDoc diffs for the unicode package:
type RangeTable struct {
    R16         []Range16
    R32         []Range32
    LatinOffset int // number of entries in R16 with Hi <= MaxLatin1
}
    RangeTable defines a set of Unicode code points by listing the ranges of
    code points within the set. The ranges are listed in two slices to save
    space: a slice of 16-bit ranges and a slice of 32-bit ranges. The two
    slices must be in sorted order and non-overlapping. Also, R32 should
    contain only values >= 0x10000 (1<<16).

func Merge(ranges ...*RangeTable) *RangeTable
    Merge returns a new RangeTable that is the union of the given tables. It
    can also be used to compact user-created RangeTables. The entries in R16
    and R32 for any given RangeTable should be sorted and non-overlapping.

    A lookup in the resulting table can be several times faster than using
    In directly on the ranges. Merge is an expensive operation, however, and
    only makes sense if one intends to use the result for more than a couple
    of hundred lookups.

func (t *RangeTable) In(r rune) bool
    In returns whether r is in t.

func (t *RangeTable) NotIn(r rune) bool
    NotIn returns whether r is not in t.

151590043:
unicode: added In and NotIn methods to RangeTables. This allows RangeTables
to directly be passed to Remove, Validate and possibly other funcs in the
go.text/runes (see https://codereview.appspot.com/151580043/) or other
go.text packages.

For example, to get a transformer that removes modifiers, one would now
write:

        isMn := func(r rune) bool { return unicode.In(r, unicode.Mn) }
        rmMn := runes.Remove(isMn)

With this change it could be rewritten as:

        rmMn := runes.Remove(unicode.Mn.In)

Note: inlining the body of Is in the new In methods seems to increase
performance by about 15%. I havent done so for clarity, but it was one
factor in the decision of adding these methods (versus adding helper funcs
in the runes package, for example) as it will allow for a performance
optimization that otherwise doesn't seem possible.

Reviewer: r@golang.org
CC: golang-codereviews@googlegroups.com, nigeltao@golang.org
Files:
src/unicode/letter.go

151660044:
unicode: added Merge func for creating RangeTables that are the union of
other range tables or just an optimized table from a non-compacted
user-created table.

Using merged tables can increase performance noticeably.

Reviewer: r@golang.org
CC: golang-codereviews@googlegroups.com, nigeltao@golang.org
Files:
src/unicode/merge.go
src/unicode/merge_test.go


On Thu, Oct 23, 2014 at 5:52 AM, Rob Pike <r@golang.org> wrote:

> Marcel: Please mail the godoc output for this package. I find that's
> easier to absorb than the code version (and I don't have to patch in your
> CL).
>
> -rob
>
>
> On Wed, Oct 22, 2014 at 8:48 PM, <nigeltao@golang.org> wrote:
>
>> LGTM, but wait for r.
>>
>> I'd still cut the API surface down, as it's easier to grow an API later
>> than to shrink one, but if both you and r are happy then I'll concede.
>>
>> As for package names, I still prefer transform.RemoveRunes and
>> transform.MapRunes over runes.Remove and runes.Map, but again, I'm happy
>> to concede the argument.
>>
>>
>> https://codereview.appspot.com/151580043/diff/90001/runes/example_test.go
>> File runes/example_test.go (right):
>>
>> https://codereview.appspot.com/151580043/diff/90001/
>> runes/example_test.go#newcode41
>> runes/example_test.go:41: rm := runes.Remove(runes.InString("aeiou"))
>> On 2014/10/22 11:22:37, mpvl wrote:
>>
>>> Please consider 151590043 and 151660044. Together they implement the
>>>
>> equivalent
>>
>>> for InString and NotInString for RangeTables.
>>>
>>
>>  I'm not averse to removing these functions, but please consider these
>>>
>> CLs.
>>
>> Even with a RangeTables.In method, I'd err on the side of leaving these
>> InString, NotInString and InFunc definitions out until there's a clear
>> need for it.
>>
>> https://codereview.appspot.com/151580043/diff/130001/
>> runes/example_test.go
>> File runes/example_test.go (right):
>>
>> https://codereview.appspot.com/151580043/diff/130001/
>> runes/example_test.go#newcode34
>> runes/example_test.go:34:
>> fmt.Println(replaceHyphens.String("a-b‐c⸗d﹣e"))
>> I can see the motivation for the runes.Remove function, which returns
>> something to put in a transform.Chain, but I'm not convinced that this
>> example is a compelling justification for the Transformer.String method
>> that you define in this CL. This snippet could just as easily be a call
>> to strings.Map from the standard library.
>>
>> https://codereview.appspot.com/151580043/diff/130001/runes/runes.go
>> File runes/runes.go (right):
>>
>> https://codereview.appspot.com/151580043/diff/130001/
>> runes/runes.go#newcode45
>> runes/runes.go:45: type Transformer struct {
>> I'm not convinced that this Transformer type (and its Bytes and Strings
>> methods) are necessary, but I'm not going to insist. (See my comment in
>> example_test.go).
>>
>> https://codereview.appspot.com/151580043/diff/130001/
>> runes/runes.go#newcode66
>> runes/runes.go:66: return Transformer{&replaceIllFormed{}}
>> You could drop the &.
>>
>> https://codereview.appspot.com/151580043/diff/130001/runes/runes_test.go
>> File runes/runes_test.go (right):
>>
>> https://codereview.appspot.com/151580043/diff/130001/
>> runes/runes_test.go#newcode146
>> runes/runes_test.go:146: 16: {
>> s/16/11/
>>
>> https://codereview.appspot.com/151580043/
>>
>
>

gobot

9 years, 4 months ago (2014-12-19 05:13:50 UTC) #16

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/151580043 is best) in the description in your
new CL.

Thanks very much.

Expand All Messages | Collapse All Messages