Permalink
Browse files

Special case Comma(math.MinInt64).

The lowest value of an int64 is itself when negated, so the rest of
the code kind of breaks down.  Short circuit it to avoid all the problems.

https://play.golang.org/p/AuAGwhJ69d

Closes: #45
  • Loading branch information...
dustin committed Jan 6, 2017
1 parent ef638b6 commit 904a49491cd5fb5cba832fac98e7e87d56b690a4
Showing with 9 additions and 0 deletions.
  1. +7 −0 comma.go
  2. +2 −0 comma_test.go
View
@@ -2,6 +2,7 @@ package humanize
import (
"bytes"
"math"
"math/big"
"strconv"
"strings"
@@ -13,6 +14,12 @@ import (
// e.g. Comma(834142) -> 834,142
func Comma(v int64) string {
sign := ""
// minin64 can't be negated to a usable value, so it has to be special cased.
if v == math.MinInt64 {
return "-9,223,372,036,854,775,808"
}

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 6, 2017

Collaborator

As an observation, this special case could've been placed above the declaration of sign variable since it's unused.

There's also a typo in "minin64".

@dmitshur

dmitshur Jan 6, 2017

Collaborator

As an observation, this special case could've been placed above the declaration of sign variable since it's unused.

There's also a typo in "minin64".

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Jan 6, 2017

Owner
@dustin

dustin via email Jan 6, 2017

Owner

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 6, 2017

Collaborator

Moving the declaration doesn't change the compiled code,

Looks like you're right. I thought it might cause an extra unnecessary allocation, but a quick benchmark
with -benchmem says there are 0 allocs/op.

@dmitshur

dmitshur Jan 6, 2017

Collaborator

Moving the declaration doesn't change the compiled code,

Looks like you're right. I thought it might cause an extra unnecessary allocation, but a quick benchmark
with -benchmem says there are 0 allocs/op.

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Jan 6, 2017

Owner
@dustin

dustin via email Jan 6, 2017

Owner

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jan 7, 2017

@shurcooL what's your logic there? why would an empty string ever cause an alloc?

@mvdan

mvdan Jan 7, 2017

@shurcooL what's your logic there? why would an empty string ever cause an alloc?

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 7, 2017

Collaborator

@mvdan I'm not very well-versed with allocations. I thought there was a chance that declaring a variable could cause it to be allocated (on the stack, in this case).

@dmitshur

dmitshur Jan 7, 2017

Collaborator

@mvdan I'm not very well-versed with allocations. I thought there was a chance that declaring a variable could cause it to be allocated (on the stack, in this case).

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jan 8, 2017

Oh. In general, by alloc people always mean the heap. That's what incurs the most runtime penalty and the GC work. -benchmem and all other alloc stuff from Go's tooling is always the heap, since it's about those two. Otherwise it would be "stack size" or something similar, which affects the compiled function.

This change is valid as far as code correctness goes, but I wouldn't fix it for performance reasons unless a benchmark showed so. And even then, it would probably be because the compiler doesn't do all the magic it could yet.

@mvdan

mvdan Jan 8, 2017

Oh. In general, by alloc people always mean the heap. That's what incurs the most runtime penalty and the GC work. -benchmem and all other alloc stuff from Go's tooling is always the heap, since it's about those two. Otherwise it would be "stack size" or something similar, which affects the compiled function.

This change is valid as far as code correctness goes, but I wouldn't fix it for performance reasons unless a benchmark showed so. And even then, it would probably be because the compiler doesn't do all the magic it could yet.

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 11, 2017

Collaborator

There's also a typo in "minin64".

This has been fixed in #64.

@dmitshur

dmitshur Nov 11, 2017

Collaborator

There's also a typo in "minin64".

This has been fixed in #64.

if v < 0 {
sign = "-"
v = 0 - v
View
@@ -20,6 +20,8 @@ func TestCommas(t *testing.T) {
{"10,001,000", Comma(10001000), "10,001,000"},
{"123,456,789", Comma(123456789), "123,456,789"},
{"maxint", Comma(9.223372e+18), "9,223,372,000,000,000,000"},
{"math.maxint", Comma(math.MaxInt64), "9,223,372,036,854,775,807"},
{"math.minint", Comma(math.MinInt64), "-9,223,372,036,854,775,808"},
{"minint", Comma(-9.223372e+18), "-9,223,372,000,000,000,000"},
{"-123,456,789", Comma(-123456789), "-123,456,789"},
{"-10,100,000", Comma(-10100000), "-10,100,000"},

0 comments on commit 904a494

Please sign in to comment.