gmane.comp.lang.go.devel

Re: code review 9643044: cmd/cgo: use intgo, not int, for string and slice structures (issue 9643044)

2013-05-23T18:41:36

i was thinking about the same clobber stack for tests, but
my version couldn't reliably reproduce the test failure.

the test code looks good to me.

i have a few questions about cmd/cgo/out.go.


https://codereview.appspot.com/9643044/diff/1/src/cmd/cgo/out.go
File src/cmd/cgo/out.go (left):

https://codereview.appspot.com/9643044/diff/1/src/cmd/cgo/out.go#oldcode940
src/cmd/cgo/out.go:940: return &Type{Size: p.PtrSize + 8, Align:
p.PtrSize, C: c("GoSlice")}
i think we should also change Size to p.PtrSize * 3.

https://codereview.appspot.com/9643044/diff/1/src/cmd/cgo/out.go
File src/cmd/cgo/out.go (right):

https://codereview.appspot.com/9643044/diff/1/src/cmd/cgo/out.go#newcode977
src/cmd/cgo/out.go:977: // The string data is 1 pointer + 1 int, but
this always
please update this comment.

https://codereview.appspot.com/9643044/diff/1/src/cmd/cgo/out.go#newcode1035
src/cmd/cgo/out.go:1035: typedef long long intgo;
on LP64 systems, seems long suffices for 64-bit int.
am i missing something?

https://coderev

code review 9643044: cmd/cgo: use intgo, not int, for string and slice structures (issue 9643044)

2013-05-23T18:19:42

Reviewers: golang-dev1,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cmd/cgo: use intgo, not int, for string and slice structures

Fixes issue 5548.

Please review this at https://codereview.appspot.com/9643044/

Affected files:
   M misc/cgo/test/cgo_test.go
   A misc/cgo/test/issue5548.go
   A misc/cgo/test/issue5548_c.c
   M src/cmd/cgo/out.go


Index: misc/cgo/test/cgo_test.go
===================================================================
--- a/misc/cgo/test/cgo_test.go
+++ b/misc/cgo/test/cgo_test.go
< at >< at > -40,5 +40,6 < at >< at >
  func Test5227(t *testing.T)                { test5227(t) }
  func TestCflags(t *testing.T)              { testCflags(t) }
  func Test5337(t *testing.T)                { test5337(t) }
+func Test5548(t *testing.T)                { test5548(t) }

  func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
Index: misc/cgo/test/issue5548.go
============================================

Re: code review 9671043: google-api-go-client: avoid reencoding parameters in th... (issue 9671043)

Francesc Campoy Flores — 2013-05-23T17:35:12

Gentle ping for Brad


On Wed, May 22, 2013 at 7:31 PM, <adg-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org> wrote:

Re: Re: code review 9355044: go.net/iana: new package (issue 9355044)

Brad Fitzpatrick — 2013-05-23T17:22:01

LGTM at least



On Thu, May 23, 2013 at 4:02 AM, <mikioh.mikioh-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

Re: code review 9605043: misc/emacs: Do not modify kill ring when programmatical... (issue 9605043)

2013-05-23T17:19:13

*** Submitted as
https://code.google.com/p/go/source/detail?r=656591eaf189 ***

misc/emacs: Do not modify kill ring when programmatically deleting text

Operations like gofmt and go-remove-unused-imports delete entire
lines of text. Previously this put them on the kill-ring,
negatively affecting user experience.

R=adonovan
CC=gobot, golang-dev
https://codereview.appspot.com/9605043

Committer: Brad Fitzpatrick <bradfitz-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org>


https://codereview.appspot.com/9605043/

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

2013-05-23T16:51:06

Hello rsc-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org, bradfitz-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org, rogpeppe-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org,
minux.ma-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

Please take another look.


https://codereview.appspot.com/9680043/

Re: code review 8248043: cmd/go: Add support for including C++ files in packages (issue 8248043)

2013-05-23T16:32:10

Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org, bradfitz-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org,
remyoudompheng-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org, minux.ma-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org, seb.binet-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org,
iant-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

Please take another look.


https://codereview.appspot.com/8248043/

Re: code review 9462044: io: Prioritize WriterTos over ReaderFroms in Copy. (issue 9462044)

2013-05-23T16:29:28

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

io: Prioritize WriterTos over ReaderFroms in Copy.

This only affects calls where both ReaderFrom and WriterTo are
implemented. WriterTo can issue one large write, while ReaderFrom must
Read until EOF, potentially reallocating when out of memory. With one
large Write, the Writer only needs to allocate once.

This also helps in ioutil.Discard since we can avoid copying memory when
the Reader implements WriterTo.

R=golang-dev, dsymonds, remyoudompheng, bradfitz
CC=golang-dev, minux.ma
https://codereview.appspot.com/9462044


https://codereview.appspot.com/9462044/

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

Rob Pike — 2013-05-23T16:28:53

Here's the argument that convinced me the new proposal is better: With
it, in the most general case (%[1]*.[2]*[3]f), it puts the arguments
to be fetched in the same order they would be fetched if the index
expressions were not there.

I'll update the CL.

-rob

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

Rob Pike — 2013-05-23T16:15:32

Let me retract that inclination, because I really am unsure. I'm still
thinking about it.

-rob

Re: code review 8248043: cmd/go: Add support for including C++ files in packages (issue 8248043)

2013-05-23T16:11:34

Hi Ian,

Thanks for the review, I've added some comments below.

On 2013/05/22 20:56:21, iant wrote:


https://codereview.appspot.com/8248043/diff/52001/misc/dist/bindist.go#newcode859

http://sourceforge.net/projects/mingw/files/Installer/mingw-get-inst/%22,
build Go



https://codereview.appspot.com/8248043/diff/52001/src/cmd/dist/build.c#newcode543
distribution itself.

Since g++ is usually installed along gcc, I thought allowing C++ to be
used in the Go standard library wouldn't hurt. Also, I didn't knew that
C++ files weren't expected in the Go distribution once support for them
was added. I'll take out that code.




https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode188
slashes
filename has
extension.  That is


https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode1706
string,


https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode1733
string) []string


https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newco

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

Rob Pike — 2013-05-23T16:03:15

I'm unsure what's right here; flags complicate things.  The current
design has the index modify its immediate predecessor, so %[2]x means,
"This spec applies to the second argument."  You are advocating that
it instead mean, "The following verb applies to the second argument."
That doesn't tell me whether the right syntax is in the presence of
flags. Currently it would be %[2]#x, but in the proposed change if the
index modifies the verb, where does that mean the index should go?
%[2]#x? %#[2]x? Either? The thing is, the entire directive applies to
the second argument, which is why it makes sense to bind it to the %.
If you bind it to the verb, it misses the point that all the other
elements of the format also apply to argument 2.

I'm inclined to leave it alone.

-rob

Re: code review 9605043: misc/emacs: Do not modify kill ring when programmatical... (issue 9605043)

2013-05-23T15:45:08

LGTM.

Sorry for the delay.


https://codereview.appspot.com/9605043/

Re: code review 9679044: go.talks: inline the reader package in each main, (issue 9679044)

2013-05-23T15:43:40

Done.

On 2013/05/23 03:24:09, adg wrote:
a




https://codereview.appspot.com/9679044/

Re: Re: code review 9692043: asn1: Stricter checks for DER encoded booleans (issue 9692043)

Rob Pike — 2013-05-23T15:42:16

Technically this could be argued as an API change, but since it's also
a specification bug and you've checked it, I'll say OK but withhold
the right to roll it back.

-rob


On Thu, May 23, 2013 at 8:28 AM,  <agl-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org> wrote:

Re: code review 9692043: asn1: Stricter checks for DER encoded booleans (issue 9692043)

2013-05-23T15:28:19

Although we should certainly generate the right bytes, I'm not sure
whether we should be absolutely strict when reading them.

None the less, I've run this over a large number of certificates and it
didn't cause any issues, so LGTM.

Have you signed the ICLA?http://golang.org/doc/contribute.html#copyright


https://codereview.appspot.com/9692043/diff/4001/src/pkg/encoding/asn1/asn1.go
File src/pkg/encoding/asn1/asn1.go (right):

https://codereview.appspot.com/9692043/diff/4001/src/pkg/encoding/asn1/asn1.go#newcode63
src/pkg/encoding/asn1/asn1.go:63:
"encoding/asn1: invalid boolean"

(The errors in this package could do with cleaning up; it's very old
code, by Go standards)

https://codereview.appspot.com/9692043/

code review 9692043: asn1: Stricter checks for DER encoded booleans (issue 9692043)

2013-05-23T14:02:19

Reviewers: golang-dev1,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

I'd like you to review this change to
https://code.google.com/p/go


Description:
asn1: Stricter checks for DER encoded booleans

According to X.690, only 0 and 255 are allowed as values
for encoded booleans. Also added some test for parsing
booleans

Please review this at https://codereview.appspot.com/9692043/

Affected files:
   src/pkg/encoding/asn1/asn1.go
   src/pkg/encoding/asn1/asn1_test.go


Index: src/pkg/encoding/asn1/asn1.go
===================================================================
old mode 100644
new mode 100755
--- a/src/pkg/encoding/asn1/asn1.go
+++ b/src/pkg/encoding/asn1/asn1.go
< at >< at > -51,7 +51,19 < at >< at >
  return
  }

-return bytes[0] != 0, nil
+// DER demands that "If the encoding represents the boolean value TRUE,
+// its single contents octet shall have all eight bits set to one."
+// Thus only 0 and 255 are valid encoded values.
+switch bytes[0] {
+case 0:
+ret = false
+case 0xff:

Re: code review 9438043: rsa: Implementation of RSASSA-PSS signature algorithm a... (issue 9438043)

2013-05-23T15:10:51

*** Submitted as
https://code.google.com/p/go/source/detail?r=0fb55e40bd0c ***

crypto/rsa: implement PSS signatures.

This change contains an implementation of the RSASSA-PSS signature
algorithm described in RFC 3447.

R=agl, agl
CC=gobot, golang-dev, r
https://codereview.appspot.com/9438043

Committer: Adam Langley <agl-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org>


https://codereview.appspot.com/9438043/

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

roger peppe — 2013-05-23T15:07:27

+1 - it seems quite a bit more intuitive.


i was thinking of it as in index into the variadic argument slice,
but the above makes sense too.

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

Rob Pike — 2013-05-23T14:18:08

Lucio suggested that too. Since we're breaking C compatibility, if we
ever had it, it could go that way. I'm not totally convinced it's
worth it, since this is only ever used to do translation tables and
they don't do much width-oriented stuff, but maybe if the facility
were nicer it would be used more. It's trickier to implement but maybe
not too much. I'll look into it.

Besides being what C uses, one-indexing feels right to me as well,
it's always the "first argument", as well as being (in Printf and
Sprintf) the index in the call.

-rob


On Thu, May 23, 2013 at 6:59 AM, minux <minux.ma-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

Re: code review 9680043: fmt.Printf: introduce notation for random access to arg... (issue 9680043)

minux — 2013-05-23T13:59:58


one-based index is also used by C.
so that we can think the 0-th parameter is taken up by the format string
itself?

i love this syntax as it's much intuitive than C's "%1$*2$d".
i'm not sure about the relative order or [] and the corresponding action
character.

given that we already are incompatible with C, how about we use:
         fmt.Sprintf("%[2]*.[1]*[3]f", 2, 6, 12.0) for
fmt.Sprintf("%6.2f", 12.0),
instead of:
         fmt.Sprintf("%[3]*[2].*[1]f", 2, 6, 12.0) for
fmt.Sprintf("%6.2f", 12.0),

so that the index will always immediately precede the the thing that it
affects.