Good catch here; calling pop() on an empty LongArray would produce a negative size and only then throw an Exception. Catching that would still mean dealing with negative size later!

One possible nitpick, now that I think about it, is that if the IllegalStateException is caught once, and pop() is called again, it will throw an IndexOutOfBoundsException just after the catch, but won't throw an IllegalStateException a second time, because size will be -1, and that won't be == 0. We might prefer size <= 0 because the size can be set externally, plus this method can make the size negative, and all sizes 0 or less are invalid here.

It's a little odd that the fix was for lastIndexOf() taking a char parameter, when it should have been long, but all of the tested values would still be valid before the fix. If some were negative or greater than 65535, then this would be a better test of the change that was just made.

If this is to be merged, a member must remember to fix these huge diffs first.

Ok, it was a mess but I think I've fixed it

Thanks. @melvilpf you should git pull this before making any further changes, otherwise I don't think it'll be able to push them due to the rebase against libgdx:master. I could be saying some wrong things but you get the memo.

At risk of repeating what I said at #7514 (comment), the comments throughout this seem more plentiful and verbose than they need to be. It's irrelevant whether a method is classic or otherwise, and there's a lot of "Test of the ... method" in here stating the obvious. Since multiple tests are performed in this test, it's not unreasonable to break it up with comments, but many would argue that unit tests are supposed to be as small as possible. Since they are run automatically, it doesn't matter much if there's loads of them.

I find this quite difficult to read and relying on the results of the previous tests weakens this. I would include {3} and {1, 2} directly in this test. obigu might agree? #7518 (comment)

I understand Spotless probably did this, but I find it very difficult to read as the - used for bullet points ends up looking like mathematical subtraction. This looks like a complete sentence on first read: Start with h = 1 2. You may wish to consider using <ol> and <li> to create a numbered list. If this was generated with AI and you haven't gone through it yourself, I urge you to just throw it away.

Also, for one thing testing hash codes this way doesn't actually provide any validation of any properties of a hashing function... for another thing, hashCode() impls in Java are usually quite simple, and that's fine for their typical usage in hash tables. The main requirement for a hash function to use in a hash table is just for the function to be fast to call (as in, a small bytecode size); most String keys aren't novel-length texts, and a LongArray used as a key in a HashMap probably won't have 1000+ items, so the actual performance of the function per item doesn't matter as much as if it can be inlined early, and the quality can be actually quite bad and still work fine in a hash table.

I think the whole hashCodeTest() should be removed.

This assertion is useless. LongArray#hashCode overrides Object#hashCode - you are comparing the same result in each case. I can change its body to this and it will still pass that part:

public int hashCode () {
	return 1;
}

Honestly any test on hashCode() is probably pointless. I say get rid of the hash code test.

Also, for one thing testing hash codes this way doesn't actually provide any validation of any properties of a hashing function... for another thing, hashCode() impls in Java are usually quite simple, and that's fine for their typical usage in hash tables. The main requirement for a hash function to use in a hash table is just for the function to be fast to call (as in, a small bytecode size); most String keys aren't novel-length texts, and a LongArray used as a key in a HashMap probably won't have 1000+ items, so the actual performance of the function per item doesn't matter as much as if it can be inlined early, and the quality can be actually quite bad and still work fine in a hash table.

I think the whole hashCodeTest() should be removed.

All these usages of the LongArray constructor that takes a long[] could be a good amount cleaner by using LongArray.with(3, 4, 5, 7) . It isn't at all needed, but I feel like with() should at least be known about, and used when possible.

I'm also reiterating Frosty-J's concern about the toString format being flexible to change; we might want to switch to {} instead of [] to work as an array literal, and append L to each number so this could go into source code. I don't think that's terribly likely, but it shouldn't be codified in a test.

How did 953da77 return? :(

oh no did it ? ://
I git pulled though that's so weird, and it doesn't appear in my commits

My advice not to waste time with it is, close this PR, which is a mess, and create a new clean one with all the changes from comments already applied.