code review 12968043: misc/emacs: Refactor coverage code into multiple functions

misc/emacs: Refactor coverage code into multiple functions

Also fixes color gradient being calculated against the wrong maximum.

[Dominik Honnef, 2013-08-15 00:23:59]

1) lisp really makes for terrible unified diffs, sorry about that
2) Brad, if you want to wait for Alan to return, I can understand that. For now
this is the last big misc/emacs CL, but I have 3 small ones (~3 line changes
each) queued up.

[Dominik Honnef, 2013-08-15 00:24:35]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[adonovan, 2013-08-15 01:44:10]

LG, but I have some questions mostly about the preexisting code.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1018: "Create a coverage overlay for a range of
coverd/uncovered code."
Can you document range and divisor?  (BTW: docstrings typically use ALL-CAPS to
refer to parameter names.)

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1022: -0.1)
Whence -0.1?

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1024: 0.8)
Whence 0.8?

Consider making a separate function from (count, divisor) to face. 
"go-coverage-face"?

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1045: (make-overlay
The rule for Lisp indentation is that all arguments are either on the same line
or each on their own line, and the first argument is generally on the same line
as the function.  So:

(overlay-put (make-overlay (point-min) (point-max))
             'face
             'go-coverage-untracked)

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1053: (let ((ranges '())
Use nil instead of '(), or just don't initialize it at all:

(let (ranges
      (max-count 0))
  ...)

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1065: (mapcar 'string-to-number rest)
Use #'string-to-number when quoting a function symbol.
(The difference isn't critical in Emacs Lisp but it helps the reader and the
compiler optimiser.)

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1070: (push
Remove linebreak.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1071: (make-go--covered
Remove linebreak.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1099: (with-current-buffer (or
Remove linebreak.

[Dominik Honnef, 2013-08-15 02:25:16]

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1018: "Create a coverage overlay for a range of
coverd/uncovered code."
On 2013/08/15 01:44:10, adonovan wrote:
> Can you document range and divisor?  (BTW: docstrings typically use ALL-CAPS
to
> refer to parameter names.)

Done. (Am aware of the convention, but this use of "range" in the docstring was
not directly related to the argument. Not mentally when I was writing it,
anyway.)

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1022: -0.1)
On 2013/08/15 01:44:10, adonovan wrote:
> Whence -0.1?

n = 1 + floor(-0.1 * 9) == 0

I.e., uncovered code.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1024: 0.8)
On 2013/08/15 01:44:10, adonovan wrote:
> Whence 0.8?
> 
> Consider making a separate function from (count, divisor) to face. 
> "go-coverage-face"?

go-coverage-face or go--coverage-face? That is, do you see any merit in making
it part of the public API? Went with go--coverage-face for now.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1024: 0.8)
On 2013/08/15 01:44:10, adonovan wrote:
> Whence 0.8?

0.8 is taken from the original algorithm in go.tools/cmd/cover. When the mode is
`set` and not `count`, all covered code uses the 8 (out of 10) intensity.

Generally this entire function is a direct translation of the algorithm used in
cmd/cover, to produce identical output.

> 
> Consider making a separate function from (count, divisor) to face. 
> "go-coverage-face"?

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1045: (make-overlay
On 2013/08/15 01:44:10, adonovan wrote:
> The rule for Lisp indentation is that all arguments are either on the same
line
> or each on their own line, and the first argument is generally on the same
line
> as the function.  So:
> 
> (overlay-put (make-overlay (point-min) (point-max))
>              'face
>              'go-coverage-untracked)
> 

Done.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1053: (let ((ranges '())
On 2013/08/15 01:44:10, adonovan wrote:
> Use nil instead of '(), or just don't initialize it at all:
> 
> (let (ranges
>       (max-count 0))
>   ...)

Done.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1065: (mapcar 'string-to-number rest)
On 2013/08/15 01:44:10, adonovan wrote:
> Use #'string-to-number when quoting a function symbol.
> (The difference isn't critical in Emacs Lisp but it helps the reader and the
> compiler optimiser.)

Done.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1070: (push
On 2013/08/15 01:44:10, adonovan wrote:
> Remove linebreak.

Done.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1071: (make-go--covered
On 2013/08/15 01:44:10, adonovan wrote:
> Remove linebreak.

Done.

https://codereview.appspot.com/12968043/diff/7001/misc/emacs/go-mode.el#newco...
misc/emacs/go-mode.el:1099: (with-current-buffer (or
On 2013/08/15 01:44:10, adonovan wrote:
> Remove linebreak.

Done.

[adonovan, 2013-08-15 02:36:12]

LGTM

I'll patch/commit it tomorrow when I'm at my workstation.

https://codereview.appspot.com/12968043/diff/12001/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

https://codereview.appspot.com/12968043/diff/12001/misc/emacs/go-mode.el#newc...
misc/emacs/go-mode.el:1026: -0.1) ;; Uncovered code, set to -0.1 so n becomes 0.
You can use single-; comments at end-of-line.

[Dominik Honnef, 2013-08-15 02:41:56]

On 2013/08/15 02:36:12, adonovan wrote:
> LGTM
> 
> I'll patch/commit it tomorrow when I'm at my workstation.
> 
> https://codereview.appspot.com/12968043/diff/12001/misc/emacs/go-mode.el
> File misc/emacs/go-mode.el (right):
> 
>
https://codereview.appspot.com/12968043/diff/12001/misc/emacs/go-mode.el#newc...
> misc/emacs/go-mode.el:1026: -0.1) ;; Uncovered code, set to -0.1 so n becomes
0.
> You can use single-; comments at end-of-line.

Noted. Already using ;; for end-of-line comments in some other places though. I
can either keep it that way or change it globally in a follow-up CL.

[adonovan, 2013-08-16 02:37:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=8345604e9545 ***

misc/emacs: Refactor coverage code into multiple functions

Also fixes color gradient being calculated against the wrong maximum.

R=adonovan
CC=golang-dev
https://codereview.appspot.com/12968043

Committer: Alan Donovan <adonovan@google.com>