applejag / typ

Generic types and functions that are missing from Go, including sets, linked lists, trees, etc.
https://pkg.go.dev/gopkg.in/typ.v4
MIT License
32 stars 2 forks source link

Added sync2.Set #32

Closed Alexamakans closed 2 years ago

Alexamakans commented 2 years ago

Thread-safe set.

Alexamakans commented 2 years ago

Would rather see the implementation make use of sync2.Map instead, as it already contains all the complex logic for having a lock-free map.

Do those complexities apply when it's used as a set? Is it mainly complex because of the dirty/readonly portion of it?

Haven't looked at it a whole lot nor thought about the issue, will check it out.

I've strictly been staying away from dependencies for this repo, even in testing, because I want this repo to be a dependency-free implementation where it's completely standalone. It's a silly constraint, but I want to stick with it.

I imagined this was the case when I saw there were none, I see now that it's written in the README.md as well. I like this constraint 👍🏻

Will look at doing it without dependencies.

  • Mix of responsibility for the sync2 package, especially considering the copied Product type and CartesianProduct function. The code better belongs inside the sets package, and given the added set type then maybe sets.Set should be an interface? Just a thought.
  • There's no good interop with sets.Set. There's a wrap method, but that's pretty clunky.

Both good points. In addition to maybe using sync2.Map instead, I will also likely look at alternative ways to make it fit better in pkg/sets.

applejag commented 2 years ago

Ok I got a plan:

Do you want to give this a shot, or shall I apply my proposed changes?

Alexamakans commented 2 years ago

Sounds good. I'll give it a shot 👍🏻

Copy-pasting for checklist:

I think the method names are good, SetDiff and SymDiff feel more like "gotta get used to it" or "find something that makes it click for you" rather than bad names hehe 😜 - [ ] probably rename some methods along the way, as I at least still always confuse SetDiff with SymDiff.

applejag commented 2 years ago

Regarding the REUSE compliance, I suggest:

As I license this under MIT, I require that you're OK with licensing your code under MIT as well

Alexamakans commented 2 years ago

Regarding the REUSE compliance, I suggest:

  • Files you've created completed from scratch, add your own copyright, e.g:
    // SPDX-FileCopyrightText: 2022 <your name>
    //
    // SPDX-License-Identifier: MIT
  • Files you've taken based on my files, add your copyright to the same that was in the forked file, e.g:
    // SPDX-FileCopyrightText: 2022 <your name>
    // SPDX-FileCopyrightText: 2022 Kalle Fagerberg
    //
    // SPDX-License-Identifier: MIT

As I license this under MIT, I require that you're OK with licensing your code under MIT as well

Not sure I have done this for every applicable file, but I think I got them all.

Alexamakans commented 2 years ago

The duplication it's complaining about is solely the test code for sync2.Set and maps.Set. Make it nicer, or is this acceptable?

applejag commented 2 years ago

The duplication it's complaining about is solely the test code for sync2.Set and maps.Set. Make it nicer, or is this acceptable?

Acceptable.