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

Map.ofEntries equivalent for fastutil maps #329

Open
viciscat opened this issue Aug 7, 2024 · 24 comments · May be fixed by #330
Open

Map.ofEntries equivalent for fastutil maps #329

viciscat opened this issue Aug 7, 2024 · 24 comments · May be fixed by #330

Comments

@viciscat
Copy link

viciscat commented Aug 7, 2024

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
Copy link
Owner

vigna commented Aug 7, 2024

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

@viciscat
Copy link
Author

viciscat commented Aug 7, 2024

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

@vigna
Copy link
Owner

vigna commented Aug 7, 2024

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
Copy link
Author

viciscat commented Aug 7, 2024

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
Copy link
Owner

vigna commented Aug 7, 2024

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
Copy link
Author

viciscat commented Aug 7, 2024

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

@viciscat
Copy link
Author

viciscat commented Aug 7, 2024

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
Copy link
Owner

vigna commented Aug 7, 2024

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
Copy link
Author

viciscat commented Aug 7, 2024

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
Copy link
Owner

vigna commented Aug 7, 2024

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
Copy link
Author

viciscat commented Aug 7, 2024

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
Copy link
Author

viciscat commented Aug 8, 2024

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
Copy link
Owner

vigna commented Aug 8, 2024

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

@viciscat
Copy link
Author

viciscat commented Aug 8, 2024

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

@vigna
Copy link
Owner

vigna commented Aug 8, 2024

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

@viciscat
Copy link
Author

viciscat commented Aug 8, 2024

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

@viciscat
Copy link
Author

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
Copy link
Owner

vigna commented Aug 11, 2024

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

@viciscat
Copy link
Author

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
Copy link
Owner

vigna commented Aug 11, 2024

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
Copy link
Author

viciscat commented Aug 11, 2024

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
Copy link
Author

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

@vigna
Copy link
Owner

vigna commented Aug 13, 2024

The problem is checking that it works everywhere.

@viciscat
Copy link
Author

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

@viciscat viciscat linked a pull request Aug 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants