Issue 4907041: code review 4907041: builtin: add documentation for builtins (Closed)
Descriptionbuiltin: add documentation for builtins
Do this by adding a fake package called builtin. At the moment, godoc will
not present this documentation because the function names are all lower case,
but there are plans to address this.
The print and println functions are undocumented here. I could be talked
into doing them but I'd rather not promote their use.
Patch Set 1 #
Total comments: 16
Patch Set 2 : diff -r af3bbd9ae031 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r af3bbd9ae031 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 4 : code review 4907041: builtin: add documentation for builtins #Patch Set 5 : diff -r 27f839c9fae4 https://go.googlecode.com/hg/ #
Total comments: 8
MessagesTotal messages: 29
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:13: // AnyType is here for the purposes of documentation only. It is a stand-in rouge double-space after period http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:16: type AnyType int did you consider using "T" as the name for this? http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:52: func len(v AnyType) int want to call out the cases for nil values? http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:72: // resulting map has length 0. The size may be omitted, in which case double space http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:74: // Chan: The channel's buffer is initialized with the specified buffer s/Chan/Channel/, like len and cap. http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:75: // capacity. If zero, or the size is omitted, the channel is double space http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:79: // The new built-in function allocates memory. The first argument is a type, double space http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:108: func close(c chan AnyType) You could add a send-only annotation here; that would reinforce what your first sentence says.
Sign in to reply to this message.
http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:16: type AnyType int The unsafe package uses "ArbitraryType", which is even longer. AnyType is clear, arguably clearer than T.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Very nice documentation, thanks! Some minors: http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:16: type AnyType int This is definitely subjective, but I find it difficult to read the functions below and think "AnyType" is necessarily the same on all uses within an individual call. It also doesn't help that we generally associate "any" with interface{}. I find "Type" or "TType" easier to read that way: func append(slice []TType, elems ...TType) []TType http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:32: // new elements. If it does not, a new slice (and underlying array) will be I believe a new slice will be allocated either way. It's just the underlying array that is reused or not. http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:41: // destination slice. (As a special case, it also will copy bytes from a I didn't know that.. good to have docs. http://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newco... src/pkg/builtin/builtin.go:50: // String: the number of byes in v. Typo
Sign in to reply to this message.
waiting for gri, since he needs to agree to the file location, name, and structure. -rob
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, n13m3y3r@gmail.com, r@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks, LGTM
Sign in to reply to this message.
LGTM I think this is fine for now. I don't care much about the location as long as it doesn't conflict with other packages. I guess builtin is fine. I need to make the respective changes to godoc. Hopefully later today (or early tomorrow). - gri
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4907041/diff/9003/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/9003/src/pkg/builtin/builtin.go#ne... src/pkg/builtin/builtin.go:19: // for any integer type: int, uint int8 etc. s/uint/uint,/ or s/,//
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=27c59a10a156 *** builtin: add documentation for builtins Do this by adding a fake package called builtin. At the moment, godoc will not present this documentation because the function names are all lower case, but there are plans to address this. The print and println functions are undocumented here. I could be talked into doing them but I'd rather not promote their use. R=dsymonds, n13m3y3r, r, gri, rsc CC=golang-dev http://codereview.appspot.com/4907041
Sign in to reply to this message.
http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... src/pkg/builtin/builtin.go:108: // clauses involving c will never execute. Humm... don't select cases involving c execute as plain send/recv on a closed chan, that is, return false/panic?
Sign in to reply to this message.
http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... src/pkg/builtin/builtin.go:126: // function (but not any function called by it) stops the panicking sequence I would add a comment that "defer panic()" does not work as expected as well. At least that's what I was trying to do several times.
Sign in to reply to this message.
On Aug 16, 2011, at 2:43 PM, dvyukov@google.com wrote: > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go > File src/pkg/builtin/builtin.go (right): > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > src/pkg/builtin/builtin.go:108: // clauses involving c will never > execute. > Humm... don't select cases involving c execute as plain send/recv on a > closed chan, that is, return false/panic? No. -rob
Sign in to reply to this message.
On Aug 16, 2011, at 2:47 PM, dvyukov@google.com wrote: > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go > File src/pkg/builtin/builtin.go (right): > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > src/pkg/builtin/builtin.go:126: // function (but not any function called > by it) stops the panicking sequence > I would add a comment that "defer panic()" does not work as expected as > well. At least that's what I was trying to do several times. Since I have no idea what I would expect that to do, I have no idea what to write. -rob
Sign in to reply to this message.
http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... src/pkg/builtin/builtin.go:49: // Slice, or map: the number of elements in v. If v is nil, len(v) is zero. Remark regarding nil values seem to duplicate the one below. http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer capacity, in units of elements. Isn't it just "buffer capacity"? Maximum capacity sounds a bit strange to me. http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer capacity, in units of elements. Don't we want to add remark regarding nil values "If v is nil, cap(v) is zero"?
Sign in to reply to this message.
On 2011/08/16 04:56:26, r2 wrote: > On Aug 16, 2011, at 2:47 PM, mailto:dvyukov@google.com wrote: > > > > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go > > File src/pkg/builtin/builtin.go (right): > > > > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > > src/pkg/builtin/builtin.go:126: // function (but not any function called > > by it) stops the panicking sequence > > I would add a comment that "defer panic()" does not work as expected as > > well. At least that's what I was trying to do several times. > > Since I have no idea what I would expect that to do, I have no idea what to > write. I expected it to stop panic if any, but it does not.
Sign in to reply to this message.
On 2011/08/16 04:50:07, r2 wrote: > On Aug 16, 2011, at 2:43 PM, mailto:dvyukov@google.com wrote: > > > > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go > > File src/pkg/builtin/builtin.go (right): > > > > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > > src/pkg/builtin/builtin.go:108: // clauses involving c will never > > execute. > > Humm... don't select cases involving c execute as plain send/recv on a > > closed chan, that is, return false/panic? > > No. Then we have serious issues in current implementation.
Sign in to reply to this message.
On Aug 16, 2011, at 2:57 PM, dvyukov@google.com wrote: > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go > File src/pkg/builtin/builtin.go (right): > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > src/pkg/builtin/builtin.go:49: // Slice, or map: the number of elements > in v. If v is nil, len(v) is zero. > Remark regarding nil values seem to duplicate the one below. No, but I will repunctuate for clarity. > > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer > capacity, in units of elements. > Isn't it just "buffer capacity"? Maximum capacity sounds a bit strange > to me. OK, although I know what I was trying to say and think it's correct. > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer > capacity, in units of elements. > Don't we want to add remark regarding nil values "If v is nil, cap(v) is > zero"? I don't believe that's implemented yet. -rob
Sign in to reply to this message.
On 2011/08/16 05:03:02, r2 wrote: > http://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#n... > > src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer > > capacity, in units of elements. > > Don't we want to add remark regarding nil values "If v is nil, cap(v) is > > zero"? > > I don't believe that's implemented yet. It works for chan/slice.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): https://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#... src/pkg/builtin/builtin.go:49: // Slice, or map: the number of elements in v. If v is nil, len(v) is zero. On 2011/08/16 04:57:33, dvyukov wrote: > Remark regarding nil values seem to duplicate the one below. Done. https://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#... src/pkg/builtin/builtin.go:59: // Channel: the maximum channel buffer capacity, in units of elements. On 2011/08/16 04:57:33, dvyukov wrote: > Isn't it just "buffer capacity"? Maximum capacity sounds a bit strange to me. Done. https://codereview.appspot.com/4907041/diff/15001/src/pkg/builtin/builtin.go#... src/pkg/builtin/builtin.go:108: // clauses involving c will never execute. On 2011/08/16 04:43:51, dvyukov wrote: > Humm... don't select cases involving c execute as plain send/recv on a closed > chan, that is, return false/panic? Acknowledged.
Sign in to reply to this message.
Message was sent while issue was closed.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go File src/pkg/builtin/builtin.go (right): https://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newc... src/pkg/builtin/builtin.go:16: type AnyType int On 2011/08/15 03:38:24, niemeyer wrote: > This is definitely subjective, but I find it difficult to read the functions > below and think "AnyType" is necessarily the same on all uses within an > individual call. It also doesn't help that we generally associate "any" with > interface{}. > > I find "Type" or "TType" easier to read that way: > > func append(slice []TType, elems ...TType) []TType Done.
Sign in to reply to this message.
Message was sent while issue was closed.
Sign in to reply to this message.
Message was sent while issue was closed.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2021/10/20 08:03:08, epaz1733 wrote:
Sign in to reply to this message.
Message was sent while issue was closed.
On 2021/10/20 08:03:08, epaz1733 wrote:
Sign in to reply to this message.
Message was sent while issue was closed.
Message package main
import (
"context"
"log"
"time"
"github.com/aws/aws-lambda-go/lambda"
)
func LongRunningHandler(ctx context.Context) (string, error) {
deadline, _ := ctx.Deadline()
deadline = deadline.Add(-100 * time.Millisecond)
timeoutChannel := time.After(time.Until(deadline))
for {
select {
case <- timeoutChannel:
return "Finished before timing out.", nil
default:
log.Print("hello!")
time.Sleep(50 * time.Millisecond)
}
}
}
func main() {
lambda.Start(LongRunningHandler)
}
https://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go
File src/pkg/builtin/builtin.go (right):
https://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newc...
src/pkg/builtin/builtin.go:16: type AnyType int
On 2011/08/15 03:31:43, r wrote:
> The unsafe package uses "ArbitraryType", which is even longer. AnyType is
clear,
> arguably clearer than T.
Done.
https://codereview.appspot.com/4907041/diff/1/src/pkg/builtin/builtin.go#newc...
src/pkg/builtin/builtin.go:50: // String: the number of byes in v.
On 2011/08/15 03:38:24, niemeyer wrote:
> Typo
Done.
Sign in to reply to this message.
|
