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

fix: address gocritic issues #127

Merged
merged 3 commits into from
Oct 4, 2022
Merged

fix: address gocritic issues #127

merged 3 commits into from
Oct 4, 2022

Conversation

yitsushi
Copy link
Contributor

@yitsushi yitsushi commented Sep 29, 2022

Example of the weird logic of slices:

package main

import "fmt"

type Data struct {
  res []int
}

func (d *Data) SetRes(res []int) {
  d.res = res
}

func (d *Data) Res() []int {
  return d.res
}

func main() {
  // New struct
  data := Data{}
  // Base data
  hist := []int{1, 2, 3, 4, 5, 6, 7}

  // Set res to be only the first 5 element of hist.
  data.SetRes(hist[:5])

  fmt.Printf("[before append] hist (%d): %+v\n", len(hist), hist)

  // Surely, appending to data.Res() and save it as 'nest' should not
  // change 'hist', right?
  nest := append(data.Res(), 8)

  // Right?
  fmt.Printf("[after append]  hist (%d): %+v\n", len(hist), hist)
  fmt.Printf("[after append]  nest (%d): %+v\n", len(nest), nest)
}

Output:

go run main.go
[before append] hist (7): [1 2 3 4 5 6 7]
[after append]  hist (7): [1 2 3 4 5 8 7]
[after append]  nest (6): [1 2 3 4 5 8]

For example:

extra = append(c.GetDescriptor().Resources, *res)

This line can lead to issues as soon as we use partial slices somewhere along the chain, it can change this in the ComponentDescriptor because GetDescriptor returns with a pointer.

Closes #85
Fixes #130

address gocritic issues

Example of the weird logic of slices:

```go
package main

import "fmt"

type Data struct {
  res []int
}

func (d *Data) SetRes(res []int) {
  d.res = res
}

func (d *Data) Res() []int {
  return d.res
}

func main() {
  // New struct
  data := Data{}
  // Base data
  hist := []int{1, 2, 3, 4, 5, 6, 7}

  // Set res to be only the first 5 element of hist.
  data.SetRes(hist[:5])

  fmt.Printf("[before append] hist (%d): %+v\n", len(hist), hist)

  // Surely, appending to data.Res() and save it as 'nest' should not
  // change 'hist', right?
  nest := append(data.Res(), 8)

  // Right?
  fmt.Printf("[after append]  hist (%d): %+v\n", len(hist), hist)
  fmt.Printf("[after append]  nest (%d): %+v\n", len(nest), nest)
}
```

Output:

```
go run main.go
[before append] hist (7): [1 2 3 4 5 6 7]
[after append]  hist (7): [1 2 3 4 5 8 7]
[after append]  nest (6): [1 2 3 4 5 8]
```

For example:
```go
extra = append(c.GetDescriptor().Resources, *res)
```

This line can lead to issues as soon as we use partial slices somewhere along
the chain, it can change this in the `ComponentDescriptor` because
`GetDescriptor` returns with a pointer.

Closes #85
Comment on lines 103 to 105
nested := make(gcommom.History, 0)
copy(nested, o.GetHistory())
nested = append(nested, key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a common problem, so we should add an appropriate method to the History type:

func (h History) Append(nv... NameVersion) History {
	return append(append(h[:0:0], h...), nv...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I wrote above append will allocate a new memory address for the return value.

About how common: I don't see it's a common problem. The code base is already huge and we had only two of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a simple method that encapsulates the semantic. And it is not necessarily the case that we will have only those two occurrences in the future. Basically, it is an intrinsic feature of a History object to be extended, therefore, there should be an appropriate method. It should just be part of the API of a History object. If you feel better with the make-based approach, I don't have a problem with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point, are you sure it works for a new slice of length 0?
The docu says:

It returns the number of elements copied, which will be the minimum of len(dst) and len(src). The result does not depend on whether the arguments overlap.

So you should pass the length of the array to copy to the make call.

BTW: This is the reason why it is always useful to encapsulate implementations in dedicated semantic methods, instead of replicating code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it is still not correct, it should be

nested := make(gcommom.History, len(hist))

Copy link
Contributor

@mandelsoft mandelsoft Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit with a fix for this. It also introduces the History append method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not really matter, but sure.

cmds/ocm/commands/ocmcmds/references/get/cmd.go Outdated Show resolved Hide resolved
@yitsushi yitsushi merged commit 0fc9de9 into main Oct 4, 2022
@mandelsoft mandelsoft deleted the 85-gocritic branch November 8, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants