code review 8357051: undo CL 8363045 / a3ce42f9748b

undo CL 8363045 / a3ce42f9748b

It changes an exported API, and breaks the build.

««« original CL description
reflect: use unsafe.Pointer in StringHeader and SliceHeader

Relates to issue 5193.

R=r
CC=golang-dev
https://codereview.appspot.com/8363045
»»»

[dsymonds, 2013-04-07 21:45:07]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-04-07 21:59:03]

LGTM

for fixing the build.  I agree with Dmitry in that we should just document
that the caller needs to retain the Data uintptr some other means while
using reflect slice/string headers.

Regardless of whether we can change the internal representation of
strings/slices in 1.x (I doubt we can,), I think it's too early to change
SliceHeader/StringHeader's public API in Go 1.1, especially this late.




On Sun, Apr 7, 2013 at 2:45 PM, <dsymonds@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> undo CL 8363045 / a3ce42f9748b
>
> It changes an exported API, and breaks the build.
>
> ««« original CL description
> reflect: use unsafe.Pointer in StringHeader and SliceHeader
>
> Relates to issue 5193.
>
> R=r
> CC=golang-dev
>
https://codereview.appspot.**com/8363045<https://codereview.appspot.com/8363045>
> »»»
>
> Please review this at
https://codereview.appspot.**com/8357051/<https://codereview.appspot.com/8357...
>
> Affected files:
>   M src/pkg/reflect/value.go
>
>
> Index: src/pkg/reflect/value.go
> ==============================**==============================**=======
> --- a/src/pkg/reflect/value.go
> +++ b/src/pkg/reflect/value.go
> @@ -910,7 +910,7 @@
>                 tt := (*sliceType)(unsafe.Pointer(v.**typ))
>                 typ := tt.elem
>                 fl |= flag(typ.Kind()) << flagKindShift
> -               val := unsafe.Pointer(uintptr(s.Data) +
> uintptr(i)*typ.size)
> +               val := unsafe.Pointer(s.Data + uintptr(i)*typ.size)
>                 return Value{typ, val, fl}
>
>         case String:
> @@ -919,7 +919,7 @@
>                 if i < 0 || i >= s.Len {
>                         panic("reflect: string index out of range")
>                 }
> -               val := *(*byte)(unsafe.Pointer(**uintptr(s.Data) +
> uintptr(i)))
> +               val := *(*byte)(unsafe.Pointer(s.Data + uintptr(i)))
>                 return Value{uint8Type, unsafe.Pointer(uintptr(val)), fl}
>         }
>         panic(&ValueError{"reflect.**Value.Index", k})
> @@ -1310,7 +1310,7 @@
>                 return uintptr(p)
>
>         case Slice:
> -               return uintptr((*SliceHeader)(v.val).**Data)
> +               return (*SliceHeader)(v.val).Data
>         }
>         panic(&ValueError{"reflect.**Value.Pointer", k})
>  }
> @@ -1565,7 +1565,7 @@
>                 }
>                 var x string
>                 val := (*StringHeader)(unsafe.**Pointer(&x))
> -               val.Data = unsafe.Pointer(uintptr(s.Data) + uintptr(beg))
> +               val.Data = s.Data + uintptr(beg)
>                 val.Len = end - beg
>                 return Value{v.typ, unsafe.Pointer(&x), v.flag}
>         }
> @@ -1579,7 +1579,7 @@
>
>         // Reinterpret as *SliceHeader to edit.
>         s := (*SliceHeader)(unsafe.Pointer(**&x))
> -       s.Data = unsafe.Pointer(uintptr(base) +
> uintptr(beg)*typ.elem.Size())
> +       s.Data = uintptr(base) + uintptr(beg)*typ.elem.Size()
>         s.Len = end - beg
>         s.Cap = cap - beg
>
> @@ -1701,14 +1701,14 @@
>  // StringHeader is the runtime representation of a string.
>  // It cannot be used safely or portably.
>  type StringHeader struct {
> -       Data unsafe.Pointer
> +       Data uintptr
>         Len  int
>  }
>
>  // SliceHeader is the runtime representation of a slice.
>  // It cannot be used safely or portably.
>  type SliceHeader struct {
> -       Data unsafe.Pointer
> +       Data uintptr
>         Len  int
>         Cap  int
>  }
> @@ -1988,7 +1988,7 @@
>
>         // Reinterpret as *SliceHeader to edit.
>         s := (*SliceHeader)(unsafe.Pointer(**&x))
> -       s.Data = unsafe_NewArray(typ.Elem().(***rtype), cap)
> +       s.Data = uintptr(unsafe_NewArray(typ.**Elem().(*rtype), cap))
>         s.Len = len
>         s.Cap = cap
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dsymonds, 2013-04-07 21:59:49]

There's still controversy, but I'll submit this anyway so we can start
from a clean base.

[dsymonds, 2013-04-07 21:59:55]

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

undo CL 8363045 / a3ce42f9748b

It changes an exported API, and breaks the build.

««« original CL description
reflect: use unsafe.Pointer in StringHeader and SliceHeader

Relates to issue 5193.

R=r
CC=golang-dev
https://codereview.appspot.com/8363045
»»»

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/8357051

[dvyukov, 2013-04-07 22:10:18]

On Sun, Apr 7, 2013 at 2:59 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> LGTM
>
> for fixing the build.  I agree with Dmitry in that we should just document
> that the caller needs to retain the Data uintptr some other means while
> using reflect slice/string headers.
>
> Regardless of whether we can change the internal representation of
> strings/slices in 1.x (I doubt we can,), I think it's too early to change
> SliceHeader/StringHeader's public API in Go 1.1, especially this late.


As far as I understand, "we can not do this in 1.x" is equivalent to
"we can not do this".
At some point we will want to change the representation, and such a
change does not look like a good reason for Go2.0. And whatever we
number the releases, the gist will be the same -- it will break user
code.
So what do you think about also adding a comment staying that this is
*internal* representation and it can change w/o notice?
Btw, are there builtin build tags for releases (+build go1.2.0)? That
would alleviate the problem.

[adg, 2013-04-07 23:27:11]

On 8 April 2013 08:10, Dmitry Vyukov <dvyukov@google.com> wrote:

> So what do you think about also adding a comment staying that this is
> *internal* representation and it can change w/o notice?
>

Even if we add it now, it wasn't there in go1.0, so we still can't change
it.


>  Btw, are there builtin build tags for releases (+build go1.2.0)? That
> would alleviate the problem.
>

There is the build tag "go1.1" that will be satisfied for any version of go
>= 1.1, but it's not documented. (
https://code.google.com/p/go/issues/detail?id=5235)

How would build tags alleviate the problem, though? We're trying to ensure
that code written for 1.0 still compiles under 1.1. Build tags still
require the author to make changes to their code to avoid breakage.

Andrew

[dvyukov, 2013-04-07 23:32:55]

On Sun, Apr 7, 2013 at 4:26 PM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 8 April 2013 08:10, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> So what do you think about also adding a comment staying that this is
>> *internal* representation and it can change w/o notice?
>
>
> Even if we add it now, it wasn't there in go1.0, so we still can't change
> it.


Yes, I understand, this is somewhat special case.


>> Btw, are there builtin build tags for releases (+build go1.2.0)? That
>> would alleviate the problem.
>
>
> There is the build tag "go1.1" that will be satisfied for any version of go
>>= 1.1, but it's not documented.
> (https://code.google.com/p/go/issues/detail?id=5235)
>
> How would build tags alleviate the problem, though? We're trying to ensure
> that code written for 1.0 still compiles under 1.1. Build tags still require
> the author to make changes to their code to avoid breakage.

At least it will be possible to create several versions of code for
different Go releases.

+cshapiro
Carl, this makes potential GC changes more problematic.

[r, 2013-04-08 01:21:10]

Our compatibility promise allows us to change things if the old
situation is broken, but my reading says that the new collector broke
the old world.

However, the case could be made that the old specification was at
fault - Data pointed to something but was not a pointer - and
therefore we could fix it. I'm not yet asserting that is the answer,
just that that case could be made.

I also have some sympathy for the story that StringHeader and
SliceHeader are internal representations that we may change. The code
already says, "StringHeader is the runtime representation of a string.
It cannot be used safely or portably."

That gives us two plausible reasons it would be OK to fix this.

-rob

[r, 2013-04-08 01:30:48]

I agree though that we need to plan this, so commentary it is. I will prep a
CL.

-rob