Issue 6903048: code review 6903048: spec: index and array/slice size constants must fit into int (Closed)
|
Created:
11 years, 4 months ago by gri Modified:
11 years, 4 months ago Reviewers:
CC:
r, rsc, iant, ken2, golang-dev Visibility:
Public. |
Descriptionspec: index and array/slice size constants must fit into an int
Patch Set 1 #Patch Set 2 : diff -r 477b2e70b12d https://code.google.com/p/go #Patch Set 3 : diff -r 477b2e70b12d https://code.google.com/p/go #Patch Set 4 : diff -r 477b2e70b12d https://code.google.com/p/go #Patch Set 5 : diff -r 477b2e70b12d https://code.google.com/p/go #Patch Set 6 : diff -r 67026ef0e79e https://code.google.com/p/go #Patch Set 7 : diff -r 67026ef0e79e https://code.google.com/p/go #Patch Set 8 : diff -r 67026ef0e79e https://code.google.com/p/go #Patch Set 9 : diff -r 67026ef0e79e https://code.google.com/p/go #Patch Set 10 : diff -r 67026ef0e79e https://code.google.com/p/go #
Total comments: 14
Patch Set 11 : diff -r 67026ef0e79e https://code.google.com/p/go #Patch Set 12 : diff -r 67026ef0e79e https://code.google.com/p/go #
Total comments: 4
Patch Set 13 : diff -r 0ce7955e8881 https://code.google.com/p/go #
Total comments: 9
Patch Set 14 : diff -r ec3ae5b98922 https://code.google.com/p/go #
Total comments: 2
Patch Set 15 : diff -r f319ec79d278 https://code.google.com/p/go #MessagesTotal messages: 20
Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org (cc: 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.
Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode819 doc/go_spec.html:819: negative <a href="#Constants">constant</a> representable by an <code>int</code> value. Perhaps s/an int value/a value of type int/ ? https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode837 doc/go_spec.html:837: Implementation restriction: A compiler may restrict the maximum pemissible s/A compiler/An implementation/ s/pemissible/permissible/ https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode839 doc/go_spec.html:839: may be restricted by the size of the address space. This restriction is too vague. We can't simply say what we do not permit. We have to also say what we require, otherwise an implementation can only permit arrays of size < 10. For example, we could say that every implementation is required to support arrays of length up to 1/2 of the maximum possible integer. Or 1/4, or whatever. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2512 doc/go_spec.html:2512: <li>a <a href="#Constants">constant</a> index must be representable by a non-negative <code>int</code> value Perhaps "must be non-negative and must be representable by a value of type int"? https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2516 doc/go_spec.html:2516: Additionally, for <code>a</code> of type <code>A</code> or <code>*A</code> I don't think you want to say "Additionally" here. It doesn't really work with the fact that this list is not merely restrictions on the index, it also says what a[x] actually means. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2646 doc/go_spec.html:2646: A <a href="#Constants">constant</a> index must be representable by a non-negative <code>int</code> value. Perhaps "must be representable by a value of type int and must be non-negative." https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode4998 doc/go_spec.html:4998: non-negative <code>int</code> value. Same change in wording.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode819 doc/go_spec.html:819: negative <a href="#Constants">constant</a> representable by an <code>int</code> value. On 2012/12/10 20:36:21, iant wrote: > Perhaps s/an int value/a value of type int/ ? Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode837 doc/go_spec.html:837: Implementation restriction: A compiler may restrict the maximum pemissible On 2012/12/10 20:36:21, iant wrote: > s/A compiler/An implementation/ > s/pemissible/permissible/ Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode839 doc/go_spec.html:839: may be restricted by the size of the address space. On 2012/12/10 20:36:21, iant wrote: > This restriction is too vague. We can't simply say what we do not permit. We > have to also say what we require, otherwise an implementation can only permit > arrays of size < 10. For example, we could say that every implementation is > required to support arrays of length up to 1/2 of the maximum possible integer. > Or 1/4, or whatever. Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2512 doc/go_spec.html:2512: <li>a <a href="#Constants">constant</a> index must be representable by a non-negative <code>int</code> value On 2012/12/10 20:36:21, iant wrote: > Perhaps "must be non-negative and must be representable by a value of type int"? Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2516 doc/go_spec.html:2516: Additionally, for <code>a</code> of type <code>A</code> or <code>*A</code> On 2012/12/10 20:36:21, iant wrote: > I don't think you want to say "Additionally" here. It doesn't really work with > the fact that this list is not merely restrictions on the index, it also says > what a[x] actually means. Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode2646 doc/go_spec.html:2646: A <a href="#Constants">constant</a> index must be representable by a non-negative <code>int</code> value. On 2012/12/10 20:36:21, iant wrote: > Perhaps "must be representable by a value of type int and must be non-negative." Done. https://codereview.appspot.com/6903048/diff/18001/doc/go_spec.html#newcode4998 doc/go_spec.html:4998: non-negative <code>int</code> value. On 2012/12/10 20:36:21, iant wrote: > Same change in wording. Done.
Sign in to reply to this message.
LGTM except for array size bit https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html#newcode839 doc/go_spec.html:839: array length further, but it must always permit at least <code>int32</code> values. I am not sure about this. If it is an array of large things, 8g is not going to let you declare many of them. For example: type T struct [ [1<<20]byte } var x [1<<20]T is not okay. I think it would be fine not to set a lower bound and say An implementation may restrict the maximum permissible array length. For instance, the maximum size of an array type may be restricted by the size of the target's virtual address space. https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, on a 64bit platform, the maximum length may be restricted by the size 64-bit
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html#newcode839 doc/go_spec.html:839: array length further, but it must always permit at least <code>int32</code> values. On 2012/12/11 15:53:08, rsc wrote: > I am not sure about this. If it is an array of large things, 8g is not going to > let you declare many of them. For example: > > type T struct [ [1<<20]byte } > var x [1<<20]T > > is not okay. > > I think it would be fine not to set a lower bound and say > > An implementation may restrict the maximum permissible array length. > For instance, the maximum size of an array type may be restricted by the > size of the target's virtual address space. This is essentially what I had at first. Reverted. https://codereview.appspot.com/6903048/diff/21003/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, on a 64bit platform, the maximum length may be restricted by the size On 2012/12/11 15:53:08, rsc wrote: > 64-bit ACK
Sign in to reply to this message.
https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by But now you are back to not setting a lower bound. That was my original complaint, and rsc mentioned it also.
Sign in to reply to this message.
Ian, do you have any suggestions on the lower bound? In general if the element size is large enough the lower bound might be 1.
Sign in to reply to this message.
I think not setting a lower bound is fine. I added the restriction such that implementations are free to enforce sensible restrictions (such as gc does now) and don't have to resort to some kind of runtime error. If an implementation interprets the absence of a lower bound as "the maximum permitted size is 10 elements" that's fine too - but nobody is going to take that compiler serious... - gri On Tue, Dec 11, 2012 at 10:27 AM, Russ Cox <rsc@golang.org> wrote: > Ian, do you have any suggestions on the lower bound? In general if the > element size is large enough the lower bound might be 1.
Sign in to reply to this message.
I still think we should have a lower bound, as we do in our other implementation restrictions. "Implementation restriction: an implementation may restrict the maximum permissible array length. For instance, the maximum size of an array type may be restricted by the size of the target's virtual address space. That said, every implementation must support arrays for which unsafe.Sizeof applied to a value of the array type will return less than 1073741824 (1G)."
Sign in to reply to this message.
https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode820 doc/go_spec.html:820: of type <code>int</code>. "representable by a value of type int" is sort of a category error. a value is a value; it's not representing anything. i think you mean stored in a variable of type int, but that sounds wrong, and the same glitch occurs elsewhere. you've added a lot more instances of this, so let's clean it up everywhere. since a type represents a range of values (we've used that idea when introducing types), how about just 'representable by an int'? or maybe 'in the range of the int type'? https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by what is this clause trying to achieve, exactly? a compile-time constraint? a run-time-constraint? mere ass-covering?
Sign in to reply to this message.
> what is this clause trying to achieve Ian wants to make sure that implementations can reject [1<<20][1<<20][1<<20]byte (no current machine can store one of those in its virtual address space) but that they cannot reject [5][5][5]byte.
Sign in to reply to this message.
No changes but some feedback. https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode820 doc/go_spec.html:820: of type <code>int</code>. On 2012/12/11 18:36:30, r wrote: > "representable by a value of type int" is sort of a category error. a value is a > value; it's not representing anything. i think you mean stored in a variable of > type int, but that sounds wrong, and the same glitch occurs elsewhere. you've > added a lot more instances of this, so let's clean it up everywhere. > > since a type represents a range of values (we've used that idea when introducing > types), how about just 'representable by an int'? or maybe 'in the range of the > int type'? I am fine with changing this, but: a) what you are suggesting is close to what I had at first, and iant objected; b) we are using the current phrase "representable by a value of type T" elsewhere fairly consistently (see Assignability, Conversions, Constant expressions, etc.). (The notions of "representable as a value of type int", "integer value", "value of integer type", and "value of int type" are all very similar in English, but they all mean different things in the spec. It's probably worthwhile making the nomenclature clear in the spec at some point but not in this CL). https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by On 2012/12/11 18:12:35, iant wrote: > But now you are back to not setting a lower bound. That was my original > complaint, and rsc mentioned it also. quoting rsc: "I think it would be fine not to set a lower bound and say..." (what I have written now). https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by On 2012/12/11 18:36:30, r wrote: > what is this clause trying to achieve, exactly? a compile-time constraint? a > run-time-constraint? mere ass-covering? It is a compile-time constraint currently implemented by 6g. The clause is merely permitting an implementation to have such a constraint (rather than having to resort to some other error - say out of memory, at run-time). So yes, "ass-covering"...
Sign in to reply to this message.
FYI https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode820 doc/go_spec.html:820: of type <code>int</code>. On 2012/12/11 19:32:55, gri wrote: > On 2012/12/11 18:36:30, r wrote: > > "representable by a value of type int" is sort of a category error. a value is > a > > value; it's not representing anything. i think you mean stored in a variable > of > > type int, but that sounds wrong, and the same glitch occurs elsewhere. you've > > added a lot more instances of this, so let's clean it up everywhere. > > > > since a type represents a range of values (we've used that idea when > introducing > > types), how about just 'representable by an int'? or maybe 'in the range of > the > > int type'? > > I am fine with changing this, but: > > a) what you are suggesting is close to what I had at first, and iant objected; > b) we are using the current phrase "representable by a value of type T" > elsewhere fairly consistently (see Assignability, Conversions, Constant > expressions, etc.). > > (The notions of "representable as a value of type int", "integer value", "value > of integer type", and "value of int type" are all very similar in English, but > they all mean different things in the spec. It's probably worthwhile making the > nomenclature clear in the spec at some point but not in this CL). I objected mainly because of the inconsistency: I saw that other places in the spec used different wording to express the concept, and I thought that we should use that same wording in this CL. If you change the rest of the spec to use some new wording, I'm OK with that. I would normally say something like "the constant must be representable in the type int" but I want the spec to be consistent in the words used for this concept. https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by On 2012/12/11 19:32:55, gri wrote: > On 2012/12/11 18:12:35, iant wrote: > > But now you are back to not setting a lower bound. That was my original > > complaint, and rsc mentioned it also. > > quoting rsc: "I think it would be fine not to set a lower bound and say..." > (what I have written now). > Sorry, you're quite right, I did misread rsc. But if we permit the implementation to have an upper bound I still want a lower bound.
Sign in to reply to this message.
Specific wording aside, it sounds like everybody is ok with the actual content of the proposed change (array/slice sizes/indices must fit into int). If so, I can update/remove some of the issues filed. I'll take silence as a yes... Apropos the implementation restriction: perhaps I should just remove it. - gri On Tue, Dec 11, 2012 at 11:50 AM, <iant@golang.org> wrote: > FYI > > > > https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html > File doc/go_spec.html (right): > > https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode820 > doc/go_spec.html:820: of type <code>int</code>. > On 2012/12/11 19:32:55, gri wrote: >> >> On 2012/12/11 18:36:30, r wrote: >> > "representable by a value of type int" is sort of a category error. > > a value is >> >> a >> > value; it's not representing anything. i think you mean stored in a > > variable >> >> of >> > type int, but that sounds wrong, and the same glitch occurs > > elsewhere. you've >> >> > added a lot more instances of this, so let's clean it up everywhere. >> > >> > since a type represents a range of values (we've used that idea when >> introducing >> > types), how about just 'representable by an int'? or maybe 'in the > > range of >> >> the >> > int type'? > > >> I am fine with changing this, but: > > >> a) what you are suggesting is close to what I had at first, and iant > > objected; >> >> b) we are using the current phrase "representable by a value of type > > T" >> >> elsewhere fairly consistently (see Assignability, Conversions, > > Constant >> >> expressions, etc.). > > >> (The notions of "representable as a value of type int", "integer > > value", "value >> >> of integer type", and "value of int type" are all very similar in > > English, but >> >> they all mean different things in the spec. It's probably worthwhile > > making the >> >> nomenclature clear in the spec at some point but not in this CL). > > > I objected mainly because of the inconsistency: I saw that other places > in the spec used different wording to express the concept, and I thought > that we should use that same wording in this CL. If you change the rest > of the spec to use some new wording, I'm OK with that. > > I would normally say something like "the constant must be representable > in the type int" but I want the spec to be consistent in the words used > for this concept. > > > https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 > doc/go_spec.html:840: For instance, the maximum size of an array type > may be restricted by > On 2012/12/11 19:32:55, gri wrote: >> >> On 2012/12/11 18:12:35, iant wrote: >> > But now you are back to not setting a lower bound. That was my > > original >> >> > complaint, and rsc mentioned it also. > > >> quoting rsc: "I think it would be fine not to set a lower bound and > > say..." >> >> (what I have written now). > > > > Sorry, you're quite right, I did misread rsc. > > But if we permit the implementation to have an upper bound I still want > a lower bound. > > https://codereview.appspot.com/6903048/
Sign in to reply to this message.
PTAL. https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/18002/doc/go_spec.html#newcode840 doc/go_spec.html:840: For instance, the maximum size of an array type may be restricted by On 2012/12/11 19:50:27, iant wrote: > On 2012/12/11 19:32:55, gri wrote: > > On 2012/12/11 18:12:35, iant wrote: > > > But now you are back to not setting a lower bound. That was my original > > > complaint, and rsc mentioned it also. > > > > quoting rsc: "I think it would be fine not to set a lower bound and say..." > > (what I have written now). > > > > Sorry, you're quite right, I did misread rsc. > > But if we permit the implementation to have an upper bound I still want a lower > bound. I've left the restriction away now. If in doubt, leave it out. We can always add it later if we find an acceptable phrasing. Hopefully we cannot mostly agree on this CL.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
LGTM we can do the wording pedantry separately https://codereview.appspot.com/6903048/diff/19004/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/19004/doc/go_spec.html#newcode820 doc/go_spec.html:820: of type <code>int</code>. i still prefer s/a value of// in this construction, throughout. but if you'd prefer we can separate that debate from this CL.
Sign in to reply to this message.
https://codereview.appspot.com/6903048/diff/19004/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/6903048/diff/19004/doc/go_spec.html#newcode820 doc/go_spec.html:820: of type <code>int</code>. On 2012/12/12 17:39:40, r wrote: > i still prefer s/a value of// in this construction, throughout. > but if you'd prefer we can separate that debate from this CL. Happy to fix, but I like to get this CL in for now. I filed Issue 4532 so we don't forget this.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=614c33a2ccd2 *** spec: index and array/slice size constants must fit into an int R=r, rsc, iant, ken CC=golang-dev https://codereview.appspot.com/6903048
Sign in to reply to this message.
|
