Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1295)

Issue 133820043: code review 133820043: runtime: adjust errorCString definition to avoid allocation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 1 month ago by rsc
Modified:
3 years, 1 month ago
Reviewers:
gobot, bradfitz
CC:
golang-codereviews, bradfitz, dfc, r
Visibility:
Public.

Description

runtime: adjust errorCString definition to avoid allocation The low-level implementation of divide on ARM assumes that it can panic with an error created by newErrorCString without allocating. If we make interface data words require pointer values, the current definition would require an allocation when stored in an interface. Changing the definition to use unsafe.Pointer instead of uintptr avoids the allocation. This change is okay because the field really is a pointer (to a C string in rodata). Update issue 8405. This should make CL 133830043 safe to try again.

Patch Set 1 #

Patch Set 2 : diff -r e3775b98a6638ee3c69763f615a3eea4c61e6485 https://code.google.com/p/go/ #

Patch Set 3 : diff -r e3775b98a6638ee3c69763f615a3eea4c61e6485 https://code.google.com/p/go/ #

Patch Set 4 : diff -r e3775b98a6638ee3c69763f615a3eea4c61e6485 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -4 lines) Patch
M src/pkg/runtime/error.go View 1 2 chunks +8 lines, -4 lines 0 comments Download

Messages

Total messages: 8
rsc
Hello golang-codereviews@googlegroups.com (cc: dfc, r), I'd like you to review this change to https://code.google.com/p/go/
3 years, 1 month ago (2014-08-24 01:58:59 UTC) #1
dfc
Is this coming via panicdivide from vlrt_arm.c ? Josh is working on transposing those functions ...
3 years, 1 month ago (2014-08-24 02:11:46 UTC) #2
rsc
On Sat, Aug 23, 2014 at 10:11 PM, Dave Cheney <dave@cheney.net> wrote: > Is this ...
3 years, 1 month ago (2014-08-24 02:13:41 UTC) #3
bradfitz
LGTM On Sat, Aug 23, 2014 at 6:58 PM, <rsc@golang.org> wrote: > Reviewers: golang-codereviews, > ...
3 years, 1 month ago (2014-08-24 02:23:02 UTC) #4
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=c506cded50be *** runtime: adjust errorCString definition to avoid allocation The low-level implementation ...
3 years, 1 month ago (2014-08-24 03:01:58 UTC) #5
gobot
This CL appears to have broken the linux-amd64 builder. See http://build.golang.org/log/6bdc32f493adfbdd06a8521174bce2732d13db7b
3 years, 1 month ago (2014-08-24 03:16:18 UTC) #6
rsc
The other linux-amd64 builder is happy.
3 years, 1 month ago (2014-08-24 03:22:33 UTC) #7
dfc
3 years, 1 month ago (2014-08-24 07:19:56 UTC) #8
Thanks Russ. I can confirm that CL 133830043 passes on arm now.

On Sun, Aug 24, 2014 at 1:22 PM, Russ Cox <rsc@golang.org> wrote:
> The other linux-amd64 builder is happy.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted