-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add generic set type #21408
Add generic set type #21408
Conversation
} | ||
|
||
// AddMultiple adds the specified elements to a set. | ||
func (s Set[T]) AddMultiple(values ...T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use Add to also add multiple? Just like you can use append adding single or multiple values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add
returns true
if the element was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can still return bool and work for both use cases:
// Add adds the specified element to a set.
// Returns true if at least one element is added; false if all the elements is already present.
func (s Set[T]) Add(value ...T) bool {
var added bool
for _, v := range value {
if _, has := s[v]; !has {
s[v] = struct{}{}
added = true
}
}
return added
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's ambiguous when reading code Add(a,b)
to set [a]
, only b
is added. What if one day there is another function Add
with comment "true if every element is added". The return value for adding multiple values is meaningless
I stand the current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's discutable but I won't block. For our use case single Add would be more than enough and easier to use imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, different frameworks have different choices. For C++ stl, pair<iterator,bool> insert(elem)
vs void insert(begin, end)
(like current approach), while in Java bool add(elem)
vs bool addAll(elemList)
(return true if the set is changed).
Personally I like the current approach 😁
Would be nice to create |
I removed the |
If for whatever reason we want to replace base struct for Set to some other instead of |
That's not possible because there is code which uses |
why .Values() is not used there? |
It copies the keys into a new array, it's inefficient |
* upstream/main: (25 commits) [skip ci] Updated translations via Crowdin Respect user's locale when rendering the date range in the repo activity page (go-gitea#21410) Consolidate more CSS colors into variables (go-gitea#21402) Add HEAD fix to gitea doctor (go-gitea#21352) Contribution guidelines (go-gitea#21425) Refactor Gitpod configuration to improve quick spin up of automated dev environments (go-gitea#21411) Support instance-wide OAuth2 applications (go-gitea#21335) Case-insensitive NuGet symbol file GUID (go-gitea#21409) Add generic set type (go-gitea#21408) Improve OAuth integration tests (go-gitea#21390) Make e-mail sanity check more precise (go-gitea#20991) Fix broken link to frontend guidelines in hacking guidelines (go-gitea#21382) Use Name instead of DisplayName in LFS Lock (go-gitea#21415) [skip ci] Updated translations via Crowdin feat(pr review): add more space on mobile (go-gitea#21326) Bump `golang.org/x/text` (go-gitea#21412) Update gitea.service (go-gitea#21399) Do DB update after merge in hammer context (go-gitea#21401) add gitpod config (go-gitea#20995) Remove cancel button in repo creation page (go-gitea#21381) ...
This PR adds a generic set type to get rid of maps used as sets.