Fix Queue bug #4300

[Darkyenus]

Also adds a new test for this edge-case (verbatim from issue) and cleans up Queue and QueueTest (in separate commits for easy review).

[gjroelofs]

Better to be verbose than to hide in one liners, also LibGDX formatter.

[gjroelofs]

Personally, I'd refrain from this pattern as it just makes the code harder to read without any performance benefits.

[Darkyenus]

I did it to be in line with libGDX's pattern of bringing object fields into method variables first before accessing them multiple times, for performance reasons (for Android, mainly, IIRC) (performance reason being, that due to multithreading, these two variants may have different semantics. It is true that JVM is free to change the semantics slightly for optimizations, I am not sure if it happens on Dalvik (or at least its earlier versions). Since this pattern is so common, I expect that it is just for this reason).

I'll let others decide on which "style" should be used, as similar pattern of inlining inc/dec operations is fairly common in libGDX's collections (just maybe not inside if()?).

[gjroelofs]

The main comment is on the if(++head ..) format which is an inc/dec operator inside the if loop.

[Darkyenus]

I have assumed so. My argument is that:

this.head++;
if(this.head == values.length) ...

requires two reads from the object, while:

if(++this.head == values.length) ...

has to read just once, which could be slightly faster, in some circumstances.

But it is not a big deal either way. I'll wait for core contributors to decide.

[RBogie]

So, I just want to jump in on this discussion.

First of all, I want to say that for readability, I prefer having the increment out of the if.

However, I mostly want to jump in because of the argument that this would create multiple reads. Fact is, you simply can't rely on what bytecode is generated for such small syntactic differences. I did test it out for the current java version on my system (javac 1.8.0_74), and when using an instance field it actually generated the exact same bytecode:

aload_0     //Load this pointer on stack
dup         //Duplicate this pointer on stack
getfield #2 // Field test:I
iconst_1    // Push 1 to stack
iadd        // Add top 2 elements
putfield #2 // Field test:I
aload_0     // Load this pointer on stack
getfield #2 // Field test:I

However, when using a static field, it would read the field only once, but duplicate it once more on the stack. It then depends on the jit compiler whether one is faster than the other. However, in the field instance case, performance will be exactly the same.

[gjroelofs]

@Darkyenus Apologies, I misinterpreted your explanation as I saw no assignment. To me, this level of optimization is rather done by the compiler if it comes a the expense of readability.

[gjroelofs]

Same as l145

[gjroelofs]

Probably better not to have ternary statements inside if statements. Better to split it up.
Also, make sure you're using the LibGDX formatter.

[Darkyenus]

This is IntelliJ autogenerated null-safe equals check. I can split it if needed, but as it is just in Test, I didn't deem that necessary.

[gjroelofs]

After rereading; I'd just use Assert.assertEquals().

[Darkyenus]

Fair points, thanks for comments, amended!

[gjroelofs]

Also; thanks for the fix :-)

[badlogic]

Thanks!