Skip to content

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Mar 28, 2025

The ArrayStore item used to emit an array store instruction based on the type kind of the array, which is always reference. It needs to do that based on the type kind of the component type of the array.

Further, this commit replaces all occurences of "element type" with "component type", because that's what they really are. This doesn't make a difference with single-dimensional arrays, which is the vast majority of arrays, but it does make a difference with multi-dimensional arrays, where component type is the type of the inner array, while the element type is never an array type.

Finally, some tests for arrays are added.

The `ArrayStore` item used to emit an array store instruction based on
the type kind of the array, which is always reference. It needs to do
that based on the type kind of the _component type_ of the array.

Further, this commit replaces all occurences of "element type" with
"component type", because that's what they really are. This doesn't
make a difference with single-dimensional arrays, which is the vast
majority of arrays, but it does make a difference with multi-dimensional
arrays, where component type is the type of the inner array, while
the element type is never an array type.

Finally, some tests for arrays are added.
@Ladicek Ladicek requested review from dmlloyd and mkouba March 28, 2025 12:41
@@ -32,6 +36,6 @@ protected Node forEachDependency(final Node node, final BiFunction<Item, Node, N
}

public void writeCode(final CodeBuilder cb, final BlockCreatorImpl block) {
cb.arrayStore(arrayExpr.typeKind());
cb.arrayStore(TypeKind.from(componentType));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual bugfix. The rest of this commit is cleanup :-)

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.

@mkouba mkouba merged commit 5661c4a into quarkusio:2.x Mar 28, 2025
2 checks passed
@Ladicek Ladicek deleted the fix-array-store-2.x branch March 28, 2025 14:10
@mkouba
Copy link
Contributor

mkouba commented Mar 28, 2025

@Ladicek @dmlloyd Speaking of arrays I find the default io.quarkus.gizmo2.Expr.elem(Constant), io.quarkus.gizmo2.Expr.elem(ConstantDesc) and io.quarkus.gizmo2.Expr.elem(Constable) methods quite useless. Note that you can't do something like expr.elem(Integer.valueOf(1)) because of ambiguity. WDYT?

@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 28, 2025

Agree that 2 variants accepting int and Expr should be enough.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 28, 2025

I would add an explicit Integer overload too, personally, otherwise we'd end up with a useless unbox-rebox cycle.

@Ladicek Ladicek added the 2.x Issue applies to Gizmo 2.x label Mar 28, 2025
@mkouba
Copy link
Contributor

mkouba commented Mar 28, 2025

Ok, so int, Integer and Expr it is. I will send a PR.

@Ladicek Ladicek moved this to Done in WG - Gizmo 2 Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Issue applies to Gizmo 2.x
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants