Code review - Issue 5011046: code review 5011046: build: add build comments to core packageshttps://codereview.appspot.com/2011-09-15T20:49:00+00:00rietveld
Message from unknown
2011-09-14T18:18:08+00:00rscurn:md5:bcf7a292d2a057d95d02c6e049482699
Message from unknown
2011-09-14T18:18:15+00:00rscurn:md5:b32331ce7f3018ce64526871c4b55691
Message from unknown
2011-09-15T16:25:52+00:00rscurn:md5:d9305247320dbb6b754969334e812949
Message from rsc@golang.org
2011-09-15T16:26:05+00:00rscurn:md5:f676a4169fd4f28abb7bb29d9439856f
Hello golang-dev (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg
Message from r@golang.org
2011-09-15T17:04:21+00:00rurn:md5:52378669f8339403290eecbee9e14e44
do they have to be
//build
as opposed to
// build
?
Message from gri@golang.org
2011-09-15T17:08:08+00:00griurn:md5:bbec5071f042024006bb6d6e5c0063d2
it does match the //line comments we already have (though I wish it
were // line, personally)
- gri
On Thu, Sep 15, 2011 at 10:04 AM, <r@golang.org> wrote:
> do they have to be
>
> //build
>
> as opposed to
>
> // build
>
> ?
>
> http://codereview.appspot.com/5011046/
>
Message from rsc@golang.org
2011-09-15T17:09:05+00:00rscurn:md5:cbc29f4bb1bae07718240f53a8539376
On Thu, Sep 15, 2011 at 13:04, <r@golang.org> wrote:
> do they have to be
>
> //build
>
> as opposed to
>
> // build
as implemented, yes, same as //line.
in both cases, it makes it unlikely that you'll just
pick off a random comment that happens
to start with a common word and start applying
meaning to it.
Message from r@google.com
2011-09-15T17:15:58+00:00r2urn:md5:00260fb70f7f68abe83362424b84d8ca
On Sep 15, 2011, at 10:07 AM, Robert Griesemer wrote:
> it does match the //line comments we already have (though I wish it
> were // line, personally)
that begs the question.
-rob
Message from rsc@golang.org
2011-09-15T17:16:41+00:00rscurn:md5:a7297c4845687e2fb77e7cda43067051
there's always // pragma line
Message from r@google.com
2011-09-15T17:16:59+00:00r2urn:md5:46b98254be6d7ce64c7e23091ab3e7f1
On Sep 15, 2011, at 10:09 AM, Russ Cox wrote:
> On Thu, Sep 15, 2011 at 13:04, <r@golang.org> wrote:
>> do they have to be
>>
>> //build
>>
>> as opposed to
>>
>> // build
>
> as implemented, yes, same as //line.
>
> in both cases, it makes it unlikely that you'll just
> pick off a random comment that happens
> to start with a common word and start applying
> meaning to it.
fair enough (although i first typed that as gsit rnouh)
-rob
Message from r@google.com
2011-09-15T17:19:42+00:00r2urn:md5:a9987dd4fb06cb182b9c0d8168d2bf4b
there's also the way the tests do it, where the first line is the magic one. makes the check for comments easier and allows more free-form commentary.
-rob
Message from rsc@golang.org
2011-09-15T17:21:56+00:00rscurn:md5:3fb9aadecdf17d9bcc9023dc95869877
On Thu, Sep 15, 2011 at 13:19, Rob 'Commander' Pike <r@google.com> wrote:
> there's also the way the tests do it, where the first line is the magic one.
true, although for //line that's not an option,
because they are by definition interspersed
with the rest of the source file.
the current CL requires that the //build comment
be before the package statement, just so you
don't have to look through the entire file.
it could require it to be the first line, although
i do like not having the copyright in the
middle of the file.
i'm inclined to leave it as //build for now,
since //line has already set a precedent.
russ
Message from r@google.com
2011-09-15T17:31:42+00:00r2urn:md5:a42c1b2f19b9562ecc72f3c693debc09
On Sep 15, 2011, at 10:21 AM, Russ Cox wrote:
> On Thu, Sep 15, 2011 at 13:19, Rob 'Commander' Pike <r@google.com> wrote:
>> there's also the way the tests do it, where the first line is the magic one.
>
> true, although for //line that's not an option,
> because they are by definition interspersed
> with the rest of the source file.
i'm not talking about //line
> the current CL requires that the //build comment
> be before the package statement, just so you
> don't have to look through the entire file.
> it could require it to be the first line, although
> i do like not having the copyright in the
> middle of the file.
for it to be in the middle of the file, the file would have to consist of one executable line after the copyright.
moreover, i don't see what this concept has to do with the package clause and therefore see little reason other than pragmatism to attach the marker there.
> i'm inclined to leave it as //build for now,
> since //line has already set a precedent.
a precedent for a completely unrelated concept, so not really a precedent at all. i think the test dir is a much more applicable precedent.
-rob
Message from gustavo@niemeyer.net
2011-09-15T17:39:01+00:00gustavo_niemeyer.neturn:md5:dcc09ed825b47a71273e913f2419fc15
> a precedent for a completely unrelated concept, so not really a precedent at all. i think the test dir is a much more applicable precedent.
In terms of precedent, there's also cgo's:
// #cgo linux CFLAGS: ...
So the analogy is not far-fetched, FWIW:
// #build linux
--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog
-- I never filed a patent.
Message from rsc@golang.org
2011-09-15T17:40:29+00:00rscurn:md5:593c6631f6f43a589e87323cb0351335
On Thu, Sep 15, 2011 at 13:38, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> In terms of precedent, there's also cgo's:
>
> // #cgo linux CFLAGS: ...
that is completely different.
no one uses // for those, and they are only
interpeted in the doc comment attached
to import "C". they are like
/*
#cgo CFLAGS: -m32
#include <stdio.h>
*/
import "C"
Message from rsc@golang.org
2011-09-15T18:15:18+00:00rscurn:md5:81a34e5539958fafd59cab9be20916c5
giving line 1 special meaning doesn't help when the
second thing like this comes along.
also people expect that the copyright notice is
boilerplate and skip over it. putting meaningful lines
above it hides them. having to add the copyright
notices to the tests exposed the problem with first-line magic.
i think the requirements for the syntax are:
(1) allow some unrelated leading comments such as copyright
(2) very low chance of falsely identifying an ordinary
comment as a directive.
(3) can be applied without parsing the file, so that
the rule could even apply to C files.
the current rule is that (rule A) in a file's
leading run of comments and blank lines,
a line beginning with //build (no /*)
is taken as a build directive.
(we've done this before with //line and //gotest,
so i didn't expect this to be contentious.)
we could change the rule to be that (rule B) in a file's
leading run of comments and blank lines
that is followed by a blank line,
a line beginning with "// build" (no /*, no tabs)
is taken as a build directive.
thus this:
---
// Copyright...
// whatever
// whatever
// build foo
package main
---
contains a build comment but this
---
// Copyright...
// whatever
// whatever
// build foo
package main
---
does not (the last few comments are not followed
by a blank line so they are not part of the leading run).
i should not have mentioned the package clause
in my last mail. it's just a way to identify 'the run of
comments and blank lines at the top of a file' in a
Go program, since the package clause is always
the first non-comment.
russ
Message from r@google.com
2011-09-15T18:24:28+00:00r2urn:md5:d4e0efce7d42f3da216d1d48d1c5f970
ok, you've convinced me. but please allow for a little more free-formness, which means not using "build" but something more specific, like "-build" or ":build", and allow spaces.
-rob
Message from unknown
2011-09-15T18:57:58+00:00rscurn:md5:3b400716028ab8f9c7b7a3d8d2b573a1
Message from rsc@golang.org
2011-09-15T18:58:05+00:00rscurn:md5:afc8faf0849d7c9f45ff906bebb3c831
Hello golang-dev@googlegroups.com, r@golang.org, gri@golang.org, r@google.com, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),
Please take another look.
Message from r@golang.org
2011-09-15T19:29:33+00:00rurn:md5:ef8a1865feeca6b938257425969ba03e
LGTM
Message from unknown
2011-09-15T20:48:41+00:00rscurn:md5:188d159f7055ac16cbc1e66ddc03cf9f
Message from rsc@golang.org
2011-09-15T20:49:00+00:00rscurn:md5:b0972aed642bfdcec92f218cbd4bf60f
*** Submitted as http://code.google.com/p/go/source/detail?r=6444b585d1ef ***
build: add build comments to core packages
The go/build package already recognizes
system-specific file names like
mycode_darwin.go
mycode_darwin_386.go
mycode_386.s
However, it is also common to write files that
apply to multiple architectures, so a recent CL added
to go/build the ability to process comments
listing a set of conditions for building. For example:
// +build darwin freebsd openbsd/386
says that this file should be compiled only on
OS X, FreeBSD, or 32-bit x86 OpenBSD systems.
These conventions are not yet documented
(hence this long CL description).
This CL adds build comments to the multi-system
files in the core library, a step toward making it
possible to use go/build to build them.
With this change go/build can handle crypto/rand,
exec, net, path/filepath, os/user, and time.
os and syscall need additional adjustments.
R=golang-dev, r, gri, r, gustavo
CC=golang-dev
http://codereview.appspot.com/5011046