Issue 7307083: code review 7307083: spec: clarify when range x does not evaluate x (Closed)
Descriptionspec: clarify when range x does not evaluate x
Fixes issue 4644.
Patch Set 1 #Patch Set 2 : diff -r e8eb2c8d5ce1 https://code.google.com/p/go/ #Patch Set 3 : diff -r e8eb2c8d5ce1 https://code.google.com/p/go/ #Patch Set 4 : diff -r e8eb2c8d5ce1 https://code.google.com/p/go/ #
Total comments: 6
Patch Set 5 : diff -r a44171137af8 https://code.google.com/p/go/ #Patch Set 6 : diff -r 18067ce7e9ba https://code.google.com/p/go/ #
Total comments: 2
Patch Set 7 : diff -r 70a50cc39caf https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 8 : diff -r eb5703eead92 https://go.googlecode.com/hg/ #MessagesTotal messages: 20
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. may be?
Sign in to reply to this message.
PTAL https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. On 2013/02/09 21:13:29, r wrote: > may be? len(x) is not always a Go constant for x of type array. len(f()) is not a constant, for example. The details are in the "#Length_and_capacity" section. Instead of linking the word length I now added an explicit section reference.
Sign in to reply to this message.
that 'and' makes it sound like it's a consequence of the range rules rather than the length. the parentheses help but a rewrite could help more.
Sign in to reply to this message.
LGTM but get other input https://codereview.appspot.com/7307083/diff/12001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/12001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length may be constant if that length is constant by definition (see ...),
Sign in to reply to this message.
This spec change should probably be accompanied by appropriate tests in $GOROOT/test/range.go. https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. This is subtle. len(e) may be constant but e may yet have side effects. For example: type A [2000000000]byte len(new(A)) // memory allocation occurs? len(<-make(chan A)) // channel created? received from? len(makeA()) // function call occurs? The wording here implies those effects are not observed but the section of the spec concerning effects says that calls and comms always occur. In practice I suspect the implied optimisation is just not very useful: if you want a loop over a static interval but don't want to materialize an array operand to 'range', then just write "for i=0; i<n; ++i". So perhaps a better choice would be to say that the expressions are evaluated in the usual way and all their effects observed in the usual order.
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. Alan, does the new wording clear this up for you? Most of the things you've listed are not constants.
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. On 2013/02/12 12:43:28, rsc wrote: > Most of the things you've listed are not constants. You're right, I didn't read your previous comment carefully enough and confused the notion of "knowable at compile-time" with "constant" as defined by the spec. (FWIW I checked go/types and it (correctly) doesn't report len(new(A)) as a constant.) What is it you're trying to convey by saying len(e) "may be constant", or that "only <something> is evaluated"? I think the two salient points are (1) that side effects in the range expression are always observed and (2) that ranging over a nil *array has len(e), not zero iterations. So how about this: "The range expression is evaluated once before beginning the loop. As a special case, if only the first iteration variable is present, and the value of the range expression e is a nil pointer to an array, then the loop has len(e) iterations, which may be more than zero."
Sign in to reply to this message.
PTAL https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/7001/doc/go_spec.html#newcode4376 doc/go_spec.html:4376: and that length may be constant. I'm not trying to say anything about the number of iterations. I think that's a distraction. I'm just trying to point out that if len(x) is a constant, then for i = range x does not evaluate x. For example, var p ***[4]int for _ = range ***p { } does not crash, because len(***p) is never evaluated. The bug you filed was that it was unclear when the expression is evaluated. That's all this text is trying to address. Please see the latest text (not the version this comment is attached to). https://codereview.appspot.com/7307083/diff/12001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/12001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length may be constant On 2013/02/11 14:29:54, r wrote: > if that length is constant by definition (see ...), Done.
Sign in to reply to this message.
On 2013/02/15 18:18:23, rsc wrote: > does not crash, because len(***p) is never evaluated. I misspoke. It does not crash because len(***p) is an official Go constant so ***p is never evaluated.
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length is constant by definition given the pointer-to-array construction, isn't the length required to be constant? perhaps all you need is s/; if/; since/ and maybe s/\.$/ in such cases./
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length is constant by definition On 2013/02/15 18:37:06, r wrote: > given the pointer-to-array construction, isn't the length required to be > constant? > perhaps all you need is > > s/; if/; since/ and maybe s/\.$/ in such cases./ No, it's not always constant. If you have func f() [4]int for i := range f() then len(f()) is not constant (spelled out in the length and capacity section) and so f does get called. But if you have var p ***[4]int for i := range **p then len(**p) is constant, so **p is never evaluated so the nil pointer deref does not happen. The exact text in the linked-to section is: The expressions <code>len(s)</code> and <code>cap(s)</code> are constants if the type of <code>s</code> is an array or pointer to an array and the expression <code>s</code> does not contain <a href="#Receive_operator">channel receives</a> or <a href="#Calls">function calls</a>; in this case <code>s</code> is not evaluated.
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length is constant by definition that's what i'm saying. if one writes "range f()" the expression is not an array, it's a function call. you've gone to all this effort to single out the case that is constant, then you say "if" it is constant.
Sign in to reply to this message.
https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/7307083/diff/14001/doc/go_spec.html#newcode4383 doc/go_spec.html:4383: length is evaluated; if that length is constant by definition On 2013/02/15 18:56:32, r wrote: > that's what i'm saying. if one writes "range f()" the expression is not an > array, it's a function call. you've gone to all this effort to single out the > case that is constant, then you say "if" it is constant. The wording here is talking about the type of the expression, not its syntax. It is selecting from the space of possible range loops the range loops over arrays. The wording here is the same as the wording at 4368: `The expression on the right in the "range" clause is called the <i>range expression</i>, which may be an array, pointer to an array, slice, string, map, or channel permitting <a href="#Receive_operator">receive operations</a>.`
Sign in to reply to this message.
LGTM i still think it's confusing but i can't find a better explanation. maybe someone else will.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=76d9c67c6646 *** spec: clarify when range x does not evaluate x Fixes issue 4644. R=r, adonovan CC=golang-dev https://codereview.appspot.com/7307083
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/02/15 19:39:32, rsc wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=76d9c67c6646 *** > > spec: clarify when range x does not evaluate x > > Fixes issue 4644. > > R=r, adonovan > CC=golang-dev > https://codereview.appspot.com/7307083 I finally understand the distinction the original spec was trying to make, so I think the current wording is an improvement. But the distinction is of such a hair-splitting nature that I doubt many users will (a) memorize it correctly and (b) use it profitably. I'd be inclined to simplify the spec in a future revision to something like: x is always evaluated (and let the compiler try to optimise around that). Do you have a particular example that motivates the current semantics?
Sign in to reply to this message.
On Mon, Feb 18, 2013 at 9:06 PM, <adonovan@google.com> wrote: > I finally understand the distinction the original spec was trying to > make, so I think the current wording is an improvement. But the > distinction is of such a hair-splitting nature that I doubt many users > will (a) memorize it correctly and (b) use it profitably. > > I'd be inclined to simplify the spec in a future revision to something > like: x is always evaluated (and let the compiler try to optimise around > that). Do you have a particular example that motivates the current > semantics? The motivation for the constant rules is to allow const x = len(...) for various reasonable ... of array type. The non-evaluation of the array during range falls out from rewriting for i := range ... as for i, n := 0, len(...); i < n; i++ (more or less). I don't see any reason to change it at this point. Implementers need to know but program authors almost never do. Except for not crashing in range *x where x is nil, it already behaves the way you say people would expect. And no one ever complains about their program not crashing. Russ
Sign in to reply to this message.
|
