|
|
|
textproto: new field serialization routine
Patch Set 1 #Patch Set 2 : diff -r 06e3a4c30dfb https://code.google.com/p/go/ #
Total comments: 3
Total messages: 8
|
pascal
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
|
13 years, 5 months ago (2012年07月21日 12:55:06 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Does this fix any issues? What's the purpose of this change? http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.go File src/pkg/net/textproto/writer.go (right): http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.... src/pkg/net/textproto/writer.go:15: const LongLineLength = 78 need these be exported constants? I don't think so
Hi Andrew, > Does this fix any issues? What's the purpose of this change? WriteMIMEField adds folding functionality which can be used for writing HTTP and SMTP headers. I shared my patch because the resolved comment in transfer.go requested it explicitly: // TODO: At some point, there should be a generic mechanism for // writing long headers, using HTTP line splitting > http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer > .go#newcode15 src/pkg/net/textproto/writer.go:15: const LongLineLength = 78 > need these be exported constants? I don't think so Unlikely indeed.
http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.go File src/pkg/net/textproto/writer.go (right): http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/textproto/writer.... src/pkg/net/textproto/writer.go:131: field := make([]byte, len(name)+len(body)+2) in the common case, this function shouldn't allocate any memory.
I'm a little skeptical that this is necessary, but it's definitely quite a bit more complex than it should be. If you can simplify it then we can look at whether it pays for itself. Right now it doesn't.
Your concerns are well understood. After a forced Google account merge I can't submit changes to this issue anymore. The attachment contains an update.
Please open a new CL if you want to continue working on this patch. You can rm $GOROOT/.hg/codereview/cl.6405072 and then run hg change to make a new one as if this one had never existed. I looked at the file, though, and it seems like it still allocates in the common case. The current code uses WriteString explicitly to avoid that problem. I'd like to understand what this does to benchmarks (I think Brad has one in the http package for this function) and I'd also like to understand how important this really is. Are there commonly used servers or clients that reject lines > 80 characters? Russ
http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/http/header.go File src/pkg/net/http/header.go (left): http://codereview.appspot.com/6405072/diff/2001/src/pkg/net/http/header.go#ol... src/pkg/net/http/header.go:96: ws, ok := w.(writeStringer) all this writeStringer stuff was done for performance reasons. I'd hate to throw that away for a corner case. If anything, only call textproto.WriteMIMEField if len(v) > textproto.MaxFieldValueLength (const) or something.