small improvements #353
small improvements #353
Conversation
|
Commits are best reviewed in isolation, although they are all pretty small. The last commit adds certain (nontrivial?) overhead, so I'd be willing to gate this behind a system property, but I found it tremendously helpful when figuring out what |
| @@ -2539,7 +2571,7 @@ default void throw_(Class<? extends Throwable> type) { | |||
| /** | |||
| * Throw a new exception of the given type with a message. | |||
| * | |||
| * @param type the exception type (must not be {@code null}) | |||
There was a problem hiding this comment.
I think this edit hit the wrong line.
There was a problem hiding this comment.
Ouch, you're right, my bad. Will fix.
| /** | ||
| * Throw a new exception of the given type with a message. | ||
| * | ||
| * @param type the exception type |
There was a problem hiding this comment.
Same here; type is not nullable but message is nullable.
| ClassDesc[] params = new ClassDesc[paramTypes.length]; | ||
| for (int i = 0; i < paramTypes.length; i++) { | ||
| params[i] = Util.classDesc(paramTypes[i]); | ||
| } |
There was a problem hiding this comment.
Maybe Stream.of(paramTypes).map(Util::classDesc).toArray(ClassDesc[]::new)?
| @@ -18,6 +18,7 @@ | |||
| import io.quarkus.gizmo2.impl.constant.ConstantImpl; | |||
|
|
|||
| public abstract non-sealed class Item implements Expr { | |||
| private final String creationSite = Util.callerOutsideGizmo(); | |||
There was a problem hiding this comment.
I wonder if this might have a significant performance impact. We might want to disable it by default.
There was a problem hiding this comment.
Right, I was expecting this comment :-) I have no idea how I would quantify the impact, but the number of Items we allocate is nontrivial, so the impact here is likely to be nontrivial as well. I'll add a system property.
The existing overloads of `newEmptyArray()` take an `Expr` as a size, but it's often necessary to create a new array of statically known size. The new overloads are easier to use in that case.
It is often necessary to build the exception message dynamically, even if the exception class is known statically. The new overloads are easier to use in such situation.
It is often needed to invoke a constructor of a dynamically created class which has a statically known parameter list. The new overloads are easier to use in such situation. It is unfortunately not possible to add an overload that would accept a `List<Class<?>>`, because that would have the same erasure as an already existing method that takes `List<ClassDesc>`.
A `ConstructorDesc` must have a return type of `void`, which most factory methods on `ConstructorDesc` guarantee, but not all of them. In this commit, we validate the return type in the constructor of `ConstructorDescImpl`, which is what all factory methods end up calling.
This method returns a helper object of class `TestClassOps` that contains the same helper methods as the `TestClassMaker` class, except they operate on the given class instead of the last accepted class. In fact, the helper methods of `TestClassMaker` now delegate to `TestClassOps` created for the last accepted class.
When the system property `gizmo.trackCreations` is set (even to an empty value, the only exception is the `false` value), all created `Item`s will carry a textual description of their creation site (which consists of the class name, method name and line number). When the Item is used on an unexpected place, that creation site description is used as part of the exception message, greatly improving the debugging experience.
Ladicek
force-pushed
the
small-improvements
branch
from
fd22082 to
131610d
Compare
|
In addition to addressing the comments, I reworked the commit that updates |
MethodTyped.parameterCount()BlockCreator.newEmptyArray()overloads that take anintsizeBlockCreator.throw_()overloads that take anExprmessageConstructorDesc.of()overloads that accept vararg parameter typesConstructorDescTestClassMaker.loadClass()Itemis used on an unexpected place