vigna / fastutil

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

Map.ofEntries equivalent for fastutil maps #329

Open viciscat opened 1 month ago

viciscat commented 1 month ago

Adding something like Map#ofEntries to the fastutil maps would be neat in my humble opinion. Right now you gotta do ugly stuff like Int2ObjectMaps.unmodifiable(new Int2ObjectOpenHashMap(Map.ofEntries(...))) I could make a PR if you don't have time to. (btw great library much love ❤️)

vigna commented 1 month ago

Good point, a PR would be welcome! I have the same problem.

viciscat commented 1 month ago

sweet! Should the methods be added in the Map interfaces or Maps (with an s) classes

vigna commented 1 month ago

Good point. I'd follow the JCF and put in in Map. But what concrete class do you intend to return, an Open2OpenHashMap in an unmodifiable wrapper?

viciscat commented 1 month ago

Not sure honestly... I'm not the data structure champion. Maybe an array implementation? I heard that it's good when you just access and don't write in it. Did not do a benchmark tho. The implementation of Map#ofEntries is just an array that alternates between key and value. Won't work for most maps (unless it's the same primitive type for the key and value)

vigna commented 1 month ago

Well, if it's immutable and holds at most 10 entries I think an ArrayMap is perfectly fine. It could even be the case of creating an ImmutableArrayMap, but that would mean another 90 classes. Maybe as an internal class of Map, just copying the necessary code from ArrayMap?

viciscat commented 1 month ago

What if it's more than 10 entries? HashMap?

viciscat commented 1 month ago

Or I could just return an ArrayMap / OpenHashMap in an unmodifiable wrapper depending on the number of entries. That would be the easiest to write.

vigna commented 1 month ago

Oh you're right. Map.of has at most 10 elements, but Map.ofEntries has variable length.

Yes, you could to ArrayMap up to, say, 8 elements, and, an OpenHashMap otherwise.

viciscat commented 1 month ago

I'm guessing tests would be wanted? I don't see any test classes for the Map interfaces. Do I create a new one? Do I do it like Primitive2PrimitiveMapTest and Primitive2ObjectMapTest?

vigna commented 1 month ago

Yes, MapTest sounds right. Like Int2IntMapTest and Object2ObjectMapTest should be enough.

Please double check that I didn't put in Maps stuff that in Java is in Map... Or we might have to rethink the position.

viciscat commented 1 month ago

Some MapTests have mentions to Maps methods. But they are present to create singletons, not actually tested. MapGenericTest uses and tests Maps stuff tho.

viciscat commented 1 month ago

Hey having a little trouble compiling a jar... Running ant jar tries to compile the drv files and I don't think that's normal 😅

vigna commented 1 month ago

No, it shouldn't. Did you make -s sources first?

viciscat commented 1 month ago

Did not include -s since it isn't specified in the readme but yes I did.

vigna commented 1 month ago

Oh yeah that's just "silent" to avoid the mess. Platform? OS?

viciscat commented 1 month ago

Windows 11 😬 I'm using WSL2 to run the linux things.

viciscat commented 1 month ago

Hey it's me again, turns out I just can't read errors. It seems some java files are not getting generated correctly? For example Byte2ByteLinkedOpenHashMap.java gets generated with just OpenHashMap.drv inside of it. It might be more WSL shenanigans but asking nonetheless in case it's a known issue. image

vigna commented 1 month ago

That scripting part is for UN*X variants. That's a WSL issue, I'm sorry but I can't help you...

viciscat commented 1 month ago

I can't seem to find anywhere in gensources.sh or the makefile how it detects and replaces the drv file that just contains the name of another drv file. Kinda hard to debug when I don't know how it's done

vigna commented 1 month ago

It might be a problem with different treatment of soft links, as the .drv files of non-plain hash maps are just soft links to the real ones.

viciscat commented 1 month ago

Hey I made this little workaround in gencsource.sh and it works great it seems. Should I commit that as well or no? (The $1 at the end of the file is replaced by $TARGET_DRV_FILE)


TARGET_DRV_FILE=$1

# A workaround for Windows and WSL. Linked files only contain the "target" file's name on windows.
# So we check the first line to see if it contains ".drv". If so replace the target drv file by that.
FIRST_LINE=$(head -n 1 $TARGET_DRV_FILE)
if [[ $FIRST_LINE == *".drv"* ]]; then
  TARGET_DRV_FILE="drv/$FIRST_LINE"
fi
viciscat commented 1 month ago

So should I keep that fix to myself or commit it?

vigna commented 1 month ago

The problem is checking that it works everywhere.

viciscat commented 1 month ago

Ah yeah that makes sense. I won't commit it and just keep it there in this issue. Might help someone