code review 763041: spec changes for panic and recover.

spec changes for panic and recover.

[r, 2010-03-25 21:26:26]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[gri, 2010-03-25 22:28:13]

Some initial comments.

My primary concern with the text as it is, is that some of it seems more
appropriately placed in the tutorial and not the spec. Or in other words, if
this panic handling were the standard way of dealing with exceptions, we
wouldn't spend that many words here.

http://codereview.appspot.com/763041/diff/1/2
File doc/go_spec.html (right):

http://codereview.appspot.com/763041/diff/1/2#newcode4516
doc/go_spec.html:4516: <h3 id="Handling_errors">Handling errors</h3>
I understand why you might not want to call this "Exception handling" (or
"Handling exceptions"), but "Handling errors" seems a wider term than I would
use here and it's misleading - this is not about dealing with general errors
that might occur at run-time. It's about panics - a severe form of run-time
errors.

How about: "Handling panics"? Since we call the activity "panicking".

Perhaps at the same time, maybe it should be called "Runtime panics" and not
"Runtime errors"?

http://codereview.appspot.com/763041/diff/1/2#newcode4529
doc/go_spec.html:4529: When a function <code>fn</code> calls <code>panic</code>,
the
why not just f ?

http://codereview.appspot.com/763041/diff/1/2#newcode4530
doc/go_spec.html:4530: execution of <code>fn</code> stops immediately.  Any
functions whose
"stops" doesn't seem the right word here; execution doesn't stop, it just
proceeds elsewhere (with the steps usually taken to return from the function)

http://codereview.appspot.com/763041/diff/1/2#newcode4537
doc/go_spec.html:4537: At that point, the program, including all other
goroutines, is
isn't "the program" simply another goroutine? maybe reformulate

http://codereview.appspot.com/763041/diff/1/2#newcode4546
doc/go_spec.html:4546: executes, it stops the panicking sequence, restoring
normal execution,
it stops the panicking sequence by restoring normal execution (?)

http://codereview.appspot.com/763041/diff/1/2#newcode4555
doc/go_spec.html:4555: Since the only code that executes while panicking is in
deferred functions,
This is true and false: Any code may be executed through a deferred function,
but at the same time, recover() will return nil unless the function that is
calling it was a deferred function. Perhaps leave away or reformulate?

http://codereview.appspot.com/763041/diff/1/2#newcode4560
doc/go_spec.html:4560: If the function <code>f</code> defined here,
For instance, if the function ...

http://codereview.appspot.com/763041/diff/1/2#newcode4567
doc/go_spec.html:4567: println("panicking with value", v)
are we using println elsewhere in the spec? use fmt.Println?

http://codereview.appspot.com/763041/diff/1/2#newcode4609
doc/go_spec.html:4609: of the deferring function, a deferred closure may modify
the function's
s/closure/function literal/

(the technical term in the spec is function literal)

http://codereview.appspot.com/763041/diff/1/2#newcode4970
doc/go_spec.html:4970: Run-time execution errors such as attempting to index an
array out
Maybe just "Execution errors such as..." to avoid run-time twice in the same
sentence?

http://codereview.appspot.com/763041/diff/1/2#newcode4971
doc/go_spec.html:4971: of bounds trigger a <i>run-time error</i> equivalent to a
call of
"equivalent" sounds like such a runtime errors is the same as any call to panic
with a non-nil argument. Maybe:

... equivalent to a call of panic with the corresponding runtime.Error
argument...

(rephrase a bit)

[r2, 2010-03-25 22:37:05]

i am avoiding the word 'exceptions' because it is has incorrect  
connotations.

[r2, 2010-03-25 22:49:22]

On Mar 25, 2010, at 3:28 PM, gri@golang.org wrote:

> Some initial comments.
>
> My primary concern with the text as it is, is that some of it seems  
> more
> appropriately placed in the tutorial and not the spec. Or in other
> words, if this panic handling were the standard way of dealing with
> exceptions, we wouldn't spend that many words here.

i understand but have left it in for now. we need to decide how much  
is written down here.
i'm happy to move it once we know where it belongs - maybe in the docs  
for runtime?

> http://codereview.appspot.com/763041/diff/1/2#newcode4567
> doc/go_spec.html:4567: println("panicking with value", v)
> are we using println elsewhere in the spec? use fmt.Println?

println is defined in the spec.  Println etc are not.

[rsc1, 2010-03-25 23:57:56]

LGTM

panic and recover are built-in functions.
This seems like the right level of detail.
They're not of some other package.

http://codereview.appspot.com/763041/diff/1/2
File doc/go_spec.html (right):

http://codereview.appspot.com/763041/diff/1/2#newcode4524
doc/go_spec.html:4524: panic(interface{})
func ?

http://codereview.appspot.com/763041/diff/9001/10001#newcode4533
doc/go_spec.html:4533: <code>fn</code> returns to its caller.  To the caller,
<code>fn</code>
s/fn/F/

http://codereview.appspot.com/763041/diff/9001/10001#newcode4629
doc/go_spec.html:4629: <p>
s/gu/gua/

http://codereview.appspot.com/763041/diff/9001/10001#newcode4968
doc/go_spec.html:4968: 
s/type/interface type/?

are we committing to interface type?
it sounds like it.

http://codereview.appspot.com/763041/diff/9001/10001#newcode4969
doc/go_spec.html:4969: <p>
s/contains/implements/

http://codereview.appspot.com/763041/diff/9001/10001#newcode4978
doc/go_spec.html:4978: <p>
Error interface {
?

[r2, 2010-03-26 00:11:32]

On Mar 25, 2010, at 4:57 PM, rsc@google.com wrote:

> LGTM
>
> panic and recover are built-in functions.
> This seems like the right level of detail.
> They're not of some other package.
>
>
>
> http://codereview.appspot.com/763041/diff/1/2
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/763041/diff/1/2#newcode4524
> doc/go_spec.html:4524: panic(interface{})
> func ?
>
> http://codereview.appspot.com/763041/diff/9001/10001#newcode4533
> doc/go_spec.html:4533: <code>fn</code> returns to its caller.  To the
> caller, <code>fn</code>
> s/fn/F/
>
> http://codereview.appspot.com/763041/diff/9001/10001#newcode4629
> doc/go_spec.html:4629: <p>
> s/gu/gua/
>
> http://codereview.appspot.com/763041/diff/9001/10001#newcode4968
> doc/go_spec.html:4968:
> s/type/interface type/?
>
> are we committing to interface type?
> it sounds like it.

i'm not committing, i'm proposing.

> http://codereview.appspot.com/763041/diff/9001/10001#newcode4969
> doc/go_spec.html:4969: <p>
> s/contains/implements/
>
> http://codereview.appspot.com/763041/diff/9001/10001#newcode4978
> doc/go_spec.html:4978: <p>
> Error interface {
> ?
>
> http://codereview.appspot.com/763041/show
>
> To unsubscribe from this group, send email to golang-dev 
> +unsubscribegooglegroups.com or reply to this email with the words  
> "REMOVE ME" as the subject.

[r2, 2010-03-26 00:11:38]

PTAL

[gri, 2010-03-26 00:26:49]

http://codereview.appspot.com/763041/diff/12001/13001
File doc/go_spec.html (right):

http://codereview.appspot.com/763041/diff/12001/13001#newcode2568
doc/go_spec.html:2568: No run-time error occurs in this case.
s/error/panic/ ?

http://codereview.appspot.com/763041/diff/12001/13001#newcode4518
doc/go_spec.html:4518: <p> Two functions, <code>panic</code> and
<code>recover</code>,
Two built-in functions, ...

(in all other sections we say built-in functions - maybe we shouldn't, but it
should be consistent)

http://codereview.appspot.com/763041/diff/12001/13001#newcode4531
doc/go_spec.html:4531: They are here now for reference and discussion.
I think the main text is fine, but the examples and associated explanations are
more elaborated I would expect in the spec.

http://codereview.appspot.com/763041/diff/12001/13001#newcode4540
doc/go_spec.html:4540: <code>F</code> returns to its caller.  To the caller,
<code>fn</code>
s/fn/F/

http://codereview.appspot.com/763041/diff/12001/13001#newcode4553
doc/go_spec.html:4553: executes, it stops the panicking sequence by restoring
normal execution,
This is only true if the recover is executed in the function that is deferred,
and not any function called by the deferred function.

Your are saying something along those lines at the bottom; I am wondering if it
should be said earlier. Here's an attempt:

The recover function allows a program to manage behavior of a panicking
goroutine. Executing a recover call inside a deferred function (but not any
function called by it) stops the panicking sequence by restoring normal
execution, and retrieves the error value passed to the call of panic. If recover
is called outside the deferred function it won't stop a panicking sequence. In
this case, and when the goroutine is not panicking, recover returns nil.

The panicking sequence can be resumed ...

http://codereview.appspot.com/763041/diff/12001/13001#newcode4632
doc/go_spec.html:4632: A deferred function that calls <code>recover</code> will
see the
With the modified text above, this now becomes just part of the example and not
part of the spec.

http://codereview.appspot.com/763041/diff/16002/19001#newcode4973
doc/go_spec.html:4973: of bounds trigger a <i>run-time error</i> equivalent to a
call of
s/error/panic/

[r2, 2010-03-26 00:52:37]

with your rewrite, we don't really need the final example, but i don't  
want to lose it so i commented it out for now.

PTAL

[gri, 2010-03-26 00:57:53]

LGTM

On Thu, Mar 25, 2010 at 5:52 PM, Rob 'Commander' Pike <r@google.com> wrote:

> with your rewrite, we don't really need the final example, but i don't want
> to lose it so i commented it out for now.
>
> PTAL
>
>
>

[r, 2010-03-26 01:00:03]

*** Submitted as http://code.google.com/p/go/source/detail?r=fff888b4a758 ***

spec changes for panic and recover.

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/763041