vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.74k stars 194 forks source link

Add ImmutableSet types #294

Closed goldfeder closed 7 months ago

goldfeder commented 1 year ago

This ought to be more or less correct (code compiles, tests pass, seems functional) but I'd appreciate your feedback!

I think I've mostly figured out how the drivers work, but I decided to start with only the immutable sets, which are relatively simple, before doing the map types.

vigna commented 1 year ago

Wow, you totally got it without my help. Kudos, as sometimes when I don't work on it for a while I don't get it. 😂

The only things I would change are:

goldfeder commented 1 year ago

Thanks! It was pretty logical and I managed with a little trial and error.

I will make the requested changes (although I could use help with the docs) but it will probably wait a week because we are on holidays here in Israel and I am not going to be at a computer much.

After this looks good I’ll send you the maps code, and also some other random fixes and utilities that you may (or may not) like. I did a bunch of stuff with strengthening Stream to work better with fastutil that I’d love to show you.

On Mon, Apr 3, 2023 at 10:07 AM Sebastiano Vigna @.***> wrote:

Wow, you totally got it without my help. Kudos, as sometimes when I don't work on it for a while I don't get it. 😂

The only things I would change are:

  • Tabs in fastutil are actual tabs (currently in the code an indentation is two spaces). But that's very easy to fix.
  • Probably we need a bit more documentation explaining in detail what is the difference with an immutable wrapper method from Sets, so people can choose what to use, as we cannot assume they are familiar with what Guava does. In theory, it would be great to have a @see in the methods of corresponding unmodifiable containers, but unfortunately one of the limitations of the method I'm using is that we don't have replacements in comments. Usually I circumvent this using some wordy description—I'll think of something.
  • The license blurb should be at the start of test files, too.

— Reply to this email directly, view it on GitHub https://github.com/vigna/fastutil/pull/294#issuecomment-1493792998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZU7OOKQQNNMK4FWV6KDYLW7JZJZANCNFSM6AAAAAAWQRR5V4 . You are receiving this because you authored the thread.Message ID: @.***>

goldfeder commented 7 months ago

Oh my goodness.

I switched jobs months and months ago and also moved and a bunch of other stuff and in the process I totally forgot about this. Months later I remembered and I was… embarrassed to reach out after having disappeared for so long. And then after that I sort of put it in the back of my mind and never contacted you.

I literally have the code ready to go. Can I finish this off? If you hadn’t closed this I probably never would have remembered but if you are still willing I can knock this out for real this week and have it submitted.

On Mon, Dec 4, 2023 at 2:22 PM Sebastiano Vigna @.***> wrote:

Closed #294 https://github.com/vigna/fastutil/pull/294.

— Reply to this email directly, view it on GitHub https://github.com/vigna/fastutil/pull/294#event-11138374467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZU7OI4EIRTIP7QM4Y6S5TYHW6BFAVCNFSM6AAAAAAWQRR5V6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRGEZTQMZXGQ2DMNY . You are receiving this because you authored the thread.Message ID: @.***>