spec: allow conversion from slice to array #46505
Comments
|
Note the I think the decision to omit direct-to-array casts was just to keep the feature change minimal. We need the cast-to-pointer regardless to support aliasing to the same backing store, and cast-to-array is easily implemented using cast-to-pointer. |
|
Some like #36890 |
|
I don't immediately see anything wrong with allowing slice to array conversions, but I feel like saving two asterisks in an already rather niche feature doesn't justify the extra complexity on compilers and tooling. |
|
Since this conversion would do a copy anyway, would we also add a conversion from |
rsc
changed the title
|
This proposal has been added to the active column of the proposals project |
|
FWIW I didn't propose this originally because it could easily be implemented, even prior to #395, by using I'm not entirely convinced that it's worth it tbh, especially given the possibility for the expression to panic. |
|
Question: is it possible to make changes to FWIW I'm a lot less concerned about a few extra characters in the line than I am about readability. I was pretty confused as a reader to see a conversion in the form |
Probably. I don't think there's any fundamental reason they'd perform differently. Can you file an issue with examples of code that you think should perform equivalently, and we can look into it for Go 1.18?
Assuming you meant "dereference this [pointer to] array", that description is pretty much how I think of it, though arguably it's more of a (checked) "assertion" (as in type assertions), as conversions generally can't fail. A slice is a pointer + dynamic length (and capacity). The conversion here is changing it to a pointer + static length, with a runtime check to ensure the lengths are compatible. |
Brad wrote a little benchmark that demonstrated some of the performance differences which probably shouldn't be different: https://play.golang.org/p/BkYM9Gbi4t2. He got the following results: You can also check out the Sum functions in https://go-review.googlesource.com/c/go/+/322329 which are good examples of code that should perform ~equivalently. |
|
@katiehockman Thanks, filed #46529. As noted in that issue, the first two functions have different semantics than the last function when |
|
ISTM that the need for a dereference is a nice visual signal to an author that |
I am personally unlikely to notice that signal among the line-noise of the rest of the expression.
Sure, but the ship has already sailed on that one. 😅 Given that we already have array values, it doesn't seem particularly more confusing to allow conversions to them. |
Looking at this again, I'm not sure why the slice part of the expression is needed at all. return *(*[Size224]byte)(sum)? And then the direct-conversion alternative becomes return ([Size224]byte)(sum)which reads a lot cleaner to me. (Without the redundant slice expression, those redundant |
It's not a matter of confusion in this case, but signalling that there is more work being done; the pointer conversion is essentially free. WRT the signalling for |
|
Strictly speaking this is unnecessary. You can always write instead of the proposed That said, copying data out from a slice into a fixed-size array is pretty common when using fixed-size arrays (like checksums etc), so I don't really see the harm in making that less star-full. /cc @robpike |
|
Any objections to adding this? |
|
I'm leaning against. I don't think adding a new conversion case is worth saving two |
|
I'm also leaning against. We've had a string of fixes to x/tools to update code for slice-to-array-pointer conversions. None of the CLs have been very involved, but just cases where we forgot to update it. I don't think this feature is going to be so frequently used to justify forcing tools authors to support both forms. |
|
Does anyone want to make an argument in favor? |
I believe that this proposal would result in improved discoverability around this kind of type conversion, when I first started writing Go (~8 years ago) I recall at one point wondering why I couldn't convert a slice to an array just like this. This seemed like the obvious way to do it. [10]int(x)At the time (even if #395 existed), it would never have occurred to me that I could do something like this: *(*[10]int)(x)I had previously been using languages without pointers and I wasn't familiar with the semantics around dereferencing or how slices relate to them. It's the kind of small thing as a beginner you don't even bother looking into when the compiler throws an error and you simply write it another way until you find out months or years later when you see it somewhere and/or learn more about pointer semantics and realise that this is possible. IE. somebody who starts learning about pointer semantics in Go can infer that Instead of framing this proposal around "saving two asterisks", I would argue that it helps discoverability around #395 because it is an obvious way to convert from a slice to an array. |
|
I think x/tools/go/ssa still needs support for this. (/cc @timothy-king) It can probably be represented as SliceToArrayPtr + UnOp/STAR. |
Converting from nil slice to zero element array is ok, so explicitly describe the behavior in the spec. For #46505 Change-Id: I68f432deb6c21a7549bf7e870185fc62504b37f7 Reviewed-on: https://go-review.googlesource.com/c/go/+/430835 TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Robert Griesemer <gri@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Auto-Submit: Robert Griesemer <gri@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
|
Change https://go.dev/cl/432735 mentions this issue: |
Use slice-to-array conversions in AddrFromSlice and (*Addr).UnmarshalBinary. This allows allows to use AddrFrom16 and drop the redundant ipv6Slice helper. For #46505 Change-Id: I0e3a7d8825ad438115b7f23ee97cc74eec41a826 Reviewed-on: https://go-review.googlesource.com/c/go/+/432735 Reviewed-by: Damien Neil <dneil@google.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Hi, masters. Please check this cl that add slice to array converstion to x/tools at go/ssa. |
|
Change https://go.dev/cl/433816 mentions this issue: |
For golang/go#46505 Change-Id: I642409e383c851277845b37dd8423dc673c12a8b Reviewed-on: https://go-review.googlesource.com/c/tools/+/433816 Run-TryBot: xie cui <523516579@qq.com> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Tim King <taking@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
|
Change https://go.dev/cl/448396 mentions this issue: |
Resend of CL 432735 (with one additional conversion that the original CL missed) after it broke the longtest builder on x/tools and was reverted in CL 433478. Now that x/tools/go/ssa has support for this, the longtest x/tools build passes as well. Use slice-to-array conversions in AddrFromSlice and (*Addr).UnmarshalBinary. This allows using AddrFrom16 and drop the redundant ipv6Slice helper. For #46505 Change-Id: I4d8084b7a97f162e4f7d685c86aac56d960ff693 Reviewed-on: https://go-review.googlesource.com/c/go/+/448396 TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>
|
CC: @tklauser @mknyszek @ianlancetaylor Hi all, I have out-of-memory (OoM) issue related to dc98ccd. The symptom is when using golangci-lint build with above commit, it will crash due to OOM. The steps to reproduce it,
Let me know if I need to open new ticket or any other information. git bisect log, OOM crash log, |
|
@shuLhan I don't understand how that change, which is quite small, could possibly have such an effect. Please open a separate issue with as much information as you can provide. Thanks. |
|
@shuLhan When you open a separate issue, please cc me. |
|
@ianlancetaylor @timothy-king After updating Go tip to e84ce08 and golangci/golangci-lint@bc333921 there is no more crash, but only panic I think I can say that this is issue in golangci-lint itself not on the Go compiler. Sorry for the noise. |
|
@shuLhan that's dominikh/go-tools#1335. Note how @dominikh maintains his own ssa implementation, so this thread does not directly affect staticcheck as much as other tools. |
|
Change https://go.dev/cl/452936 mentions this issue: |
Document that a slice can be converted to either an array or a pointer to an array of a matching underlying array type. This was documented in the "Conversions from slice to array or array pointer" subsection, but not in the list of conversion rules. Updates #46505. Change-Id: I16a89a63ef23c33580129952415e977a8f334009 Reviewed-on: https://go-review.googlesource.com/c/go/+/452936 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Tim King <taking@google.com>
|
I believe this has been done since https://go.dev/cl/433816 landed. |
* fix array conversion that was necessary due to golang/go#46505 * preallocate slice Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
* fix array conversion that was necessary due to golang/go#46505 * preallocate slice Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
and others
I recently tried to use the new #395 feature (for converting a slice to an array pointer: https://golang.org/cl/216424) in:
https://go-review.googlesource.com/c/go/+/322329
But in review, it was pointed out that it was a little ugly, as what I wanted to return was an array, which required a dereference:
It would've been nicer if I could've just converted to an array instead:
Talking to @ianlancetaylor and @griesemer, we couldn't remember great reasons for not also allowing this as part of #395. It does mean there's an subtle copy, but arguably the
*dereference above is also a somewhat subtle copy.Could we also add support for converting to the array?
/cc @katiehockman @josharian @rogpeppe @mdempsky
The text was updated successfully, but these errors were encountered: