rpuch commented on code in PR #4152:
URL: https://github.com/apache/ignite-3/pull/4152#discussion_r1695643864
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageSerializerGenerator.java:
##########
@@ -140,19 +144,25 @@ private MethodSpec writeMessageMethod(MessageClass
message) {
return method.build();
}
- /**
- * Helper method for resolving a {@link MessageWriter} "write*" call based
on the message field type.
- */
+ /** Helper method for resolving a {@link MessageWriter} "write*" call
based on the message field type. */
private CodeBlock writeMessageCodeBlock(ExecutableElement getter) {
- var methodResolver = new
MessageWriterMethodResolver(processingEnvironment);
-
- CodeBlock writerMethodCall = CodeBlock.builder()
- .add("boolean written = writer.")
- .add(methodResolver.resolveWriteMethod(getter))
- .build();
+ CodeBlock.Builder writerMethodCallBuilder = CodeBlock.builder();
+
+ if (typeUtils.isEnum(getter.getReturnType())) {
+ String fieldName = getter.getSimpleName().toString();
+
+ writerMethodCallBuilder
+ .add("int ordinal = message.$L() == null ? 0 :
message.$L().ordinal() + 1;", fieldName, fieldName)
Review Comment:
Please add a comment explaining why we need to do `+1` (probably because we
use 0 to pass `null` and need to shift 'real' values?)
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageDeserializerGenerator.java:
##########
@@ -45,23 +50,25 @@
* Class for generating {@link MessageDeserializer} classes.
*/
public class MessageDeserializerGenerator {
+ private static final String FROM_ORDINAL_METHOD_NAME = "fromOrdinal";
+
/** Processing environment. */
private final ProcessingEnvironment processingEnv;
- /**
- * Message Types declarations for the current module.
- */
+ /** Wrapper element with {@link MessageGroup}. */
Review Comment:
Previous javadoc was telling something that the new one does not, and new
javadoc just states something that is already obvious from the field type. I'd
suggest to revert this change.
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/CompoundConditionType.java:
##########
@@ -17,10 +17,20 @@
package org.apache.ignite.internal.metastorage.dsl;
+import org.jetbrains.annotations.Nullable;
+
/**
* Type of compound condition.
*/
public enum CompoundConditionType {
AND,
- OR
+ OR;
+
+ /** Cached array with all enum values. */
+ private static final CompoundConditionType[] VALUES = values();
+
+ /** Returns the enumerated value from its ordinal, {@code null} if the
ordinal is invalid. */
+ public static @Nullable CompoundConditionType fromOrdinal(int ordinal) {
+ return ordinal < 0 || ordinal >= VALUES.length ? null :
VALUES[ordinal];
Review Comment:
Negative ordinal is never valid. We should throw something (like
`IllegalArgumentException`) for such an ordinal.
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageDeserializerGenerator.java:
##########
@@ -181,9 +186,33 @@ private CodeBlock readMessageCodeBlock(ExecutableElement
getter) {
varType = TypeName.get(getter.getReturnType());
}
- return CodeBlock.builder()
- .add("$T tmp = reader.", varType)
- .add(methodResolver.resolveReadMethod(getter))
- .build();
+ if (typeUtils.isEnum(getter.getReturnType())) {
+ checkFromOrdinalMethodExists(getter.getReturnType());
+
+ return CodeBlock.builder()
+ .add("int ordinal = reader.readInt($S);",
getter.getSimpleName().toString())
+ .add("\n")
+ .add("$T tmp = ordinal <= 0 ? null : $T.$L(ordinal - 1)",
varType, varType, FROM_ORDINAL_METHOD_NAME)
Review Comment:
Please add a comment explaining why we need to do `-1` (probably because we
use 0 to pass `null` and need to shift 'real' values?)
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageDeserializerGenerator.java:
##########
@@ -181,9 +186,33 @@ private CodeBlock readMessageCodeBlock(ExecutableElement
getter) {
varType = TypeName.get(getter.getReturnType());
}
- return CodeBlock.builder()
- .add("$T tmp = reader.", varType)
- .add(methodResolver.resolveReadMethod(getter))
- .build();
+ if (typeUtils.isEnum(getter.getReturnType())) {
+ checkFromOrdinalMethodExists(getter.getReturnType());
+
+ return CodeBlock.builder()
+ .add("int ordinal = reader.readInt($S);",
getter.getSimpleName().toString())
+ .add("\n")
+ .add("$T tmp = ordinal <= 0 ? null : $T.$L(ordinal - 1)",
varType, varType, FROM_ORDINAL_METHOD_NAME)
Review Comment:
Why is `ordinal <= 0` needed here?
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/CompoundConditionType.java:
##########
@@ -17,10 +17,20 @@
package org.apache.ignite.internal.metastorage.dsl;
+import org.jetbrains.annotations.Nullable;
+
/**
* Type of compound condition.
*/
public enum CompoundConditionType {
AND,
- OR
+ OR;
+
+ /** Cached array with all enum values. */
+ private static final CompoundConditionType[] VALUES = values();
+
+ /** Returns the enumerated value from its ordinal, {@code null} if the
ordinal is invalid. */
+ public static @Nullable CompoundConditionType fromOrdinal(int ordinal) {
Review Comment:
Why do we return `null` for an invalid ordinal? Should we throw an exception
instead?
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageDeserializerGenerator.java:
##########
@@ -181,9 +186,33 @@ private CodeBlock readMessageCodeBlock(ExecutableElement
getter) {
varType = TypeName.get(getter.getReturnType());
}
- return CodeBlock.builder()
- .add("$T tmp = reader.", varType)
- .add(methodResolver.resolveReadMethod(getter))
- .build();
+ if (typeUtils.isEnum(getter.getReturnType())) {
+ checkFromOrdinalMethodExists(getter.getReturnType());
+
+ return CodeBlock.builder()
+ .add("int ordinal = reader.readInt($S);",
getter.getSimpleName().toString())
+ .add("\n")
+ .add("$T tmp = ordinal <= 0 ? null : $T.$L(ordinal - 1)",
varType, varType, FROM_ORDINAL_METHOD_NAME)
+ .build();
+ } else {
+ return CodeBlock.builder()
+ .add("$T tmp = reader.", varType)
+ .add(methodResolver.resolveReadMethod(getter))
+ .build();
+ }
+ }
+
+ private void checkFromOrdinalMethodExists(TypeMirror enumType) {
+ assert typeUtils.isEnum(enumType) : enumType;
+
+ typeUtils.types().asElement(enumType).getEnclosedElements().stream()
+ .filter(element -> element.getKind() == ElementKind.METHOD)
+ .filter(element ->
element.getSimpleName().toString().equals(FROM_ORDINAL_METHOD_NAME))
+ .filter(element ->
element.getModifiers().contains(Modifier.PUBLIC))
+ .filter(element ->
element.getModifiers().contains(Modifier.STATIC))
+ .findAny()
+ .orElseThrow(() -> new ProcessingException(
+ String.format("Missing public static method \"%s\" for
enum %s", FROM_ORDINAL_METHOD_NAME, enumType)
+ ));
}
-}
+}
Review Comment:
Please add a trailing new line
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageDeserializerGenerator.java:
##########
@@ -181,9 +186,33 @@ private CodeBlock readMessageCodeBlock(ExecutableElement
getter) {
varType = TypeName.get(getter.getReturnType());
}
- return CodeBlock.builder()
- .add("$T tmp = reader.", varType)
- .add(methodResolver.resolveReadMethod(getter))
- .build();
+ if (typeUtils.isEnum(getter.getReturnType())) {
+ checkFromOrdinalMethodExists(getter.getReturnType());
+
+ return CodeBlock.builder()
+ .add("int ordinal = reader.readInt($S);",
getter.getSimpleName().toString())
+ .add("\n")
+ .add("$T tmp = ordinal <= 0 ? null : $T.$L(ordinal - 1)",
varType, varType, FROM_ORDINAL_METHOD_NAME)
Review Comment:
Also, why do we need a special method? Can't we just use
`<Type>.values()[ordinal]`?
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/SimpleCondition.java:
##########
@@ -29,16 +29,7 @@ public interface SimpleCondition extends Condition {
ByteBuffer key();
/** Condition type. */
- int conditionType();
-
- /**
- * Returns condition type.
- *
- * @return Condition type.
- */
- default ConditionType type() {
- return ConditionType.values()[conditionType()];
- }
+ ConditionType conditionType();
Review Comment:
It seems logical to leave `type()` as the name of this method: `type()` was
returning `ConditionType` before, and the method is defined on a class named
`SimpleCondition`, so if it's `type()`, it's already obvious that it's a
*condition* type.
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/entity/EnumOutter.java:
##########
@@ -19,7 +19,7 @@
package org.apache.ignite.raft.jraft.entity;
-public final class EnumOutter {
+import org.jetbrains.annotations.Nullable;public final class EnumOutter {
Review Comment:
Please place `import` on its own line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
