Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when shrinking a slice property #287

Closed
gjabell opened this issue Dec 22, 2021 · 2 comments · Fixed by #293
Closed

Error when shrinking a slice property #287

gjabell opened this issue Dec 22, 2021 · 2 comments · Fixed by #293

Comments

@gjabell
Copy link
Contributor

gjabell commented Dec 22, 2021

I am writing a dbus server which exposes an ObjectPath slice as a property. Adding a new element to the slice and calling prop.Set works as expected, but trying to remove an element from the slice fails with: dbus.Store: type mismatch: slices are different lengths need: 0 have: 1 due to this check:

dbus/dbus.go

Lines 281 to 286 in 8e438d3

if dest.Len() != src.Len() {
return fmt.Errorf(
"dbus.Store: type mismatch: "+
"slices are different lengths "+
"need: %d have: %d",
src.Len(), dest.Len())
.

Is there a reason why this check exists? The behavior seems to work correctly with the following change:

diff --git i/dbus.go w/dbus.go
index e9d014f..32ab49f 100644
--- i/dbus.go
+++ w/dbus.go
@@ -275,16 +275,9 @@ func storeSliceIntoInterface(dest, src reflect.Value) error {
 }
 
 func storeSliceIntoSlice(dest, src reflect.Value) error {
-	if dest.IsNil() || dest.Len() < src.Len() {
+	if dest.IsNil() || dest.Len() != src.Len() {
 		dest.Set(reflect.MakeSlice(dest.Type(), src.Len(), src.Cap()))
 	}
-	if dest.Len() != src.Len() {
-		return fmt.Errorf(
-			"dbus.Store: type mismatch: "+
-				"slices are different lengths "+
-				"need: %d have: %d",
-			src.Len(), dest.Len())
-	}
 	for i := 0; i < src.Len(); i++ {
 		err := store(dest.Index(i), getVariantValue(src.Index(i)))
 		if err != nil {

I can open a PR to fix this if that is a valid change, otherwise some guidance on how to do this properly would be appreciated.

Thanks!

@guelfey
Copy link
Member

guelfey commented Jan 5, 2022

Nice catch! Indeed, in this case we don't even need to call reflect.MakeSlice again, but could just fill dest and reslice it with reflect.Slice to avoid some allocation. Feel free to open a PR for that.

@gjabell
Copy link
Contributor Author

gjabell commented Jan 6, 2022

Thanks for the feedback :) I opened a PR to fix this; not sure if I interpreted your comment correctly so please let me know if I need to change something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants