-
Notifications
You must be signed in to change notification settings - Fork 928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-5266 code gardening after move to Java 17 #5477
base: main
Are you sure you want to change the base?
Conversation
Full test-suite is good on this. |
@@ -327,7 +326,7 @@ public void containsEmptySet() { | |||
final LongHashSet other = new LongHashSet(100); | |||
|
|||
assertTrue(testSet.containsAll(other)); | |||
assertTrue(testSet.containsAll((Collection<?>) other)); | |||
assertTrue(testSet.containsAll(other)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't redundant casts. The class has its own containsAll(LongHashSet) but really has 2 methods as it also inherits containsAll(Collection) (unclear if it shouldnt be overriding it).
Notice all the changed lines result in duplicate assertions; this cast is the test trying to force which method is used in order to check both methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -147,7 +147,7 @@ public void testInterruptedLargeMessage() throws Exception { | |||
AmqpReceiver receiver = session.createReceiver(getQueueName()); | |||
|
|||
int received = 0; | |||
receiver.flow((int) (messageCount + 10)); | |||
receiver.flow((messageCount + 10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also lose the second set of parentheses that were needed for the cast that was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -40,7 +40,6 @@ public Object createObject() throws Exception { | |||
@Override | |||
protected void populateObject(Object object) throws Exception { | |||
super.populateObject(object); | |||
ActiveMQBytesMessage info = (ActiveMQBytesMessage) object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removal then also renders the entire method superflous since all it does is call super.
I'd either leave it as it is, since these are originally generated files from ported tests (so the more we change them, the further from the originals they go)...or delete the methods instead of just the variable.
Same goes for the likely tens of other similar cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the relevant changes in that module.
@@ -424,8 +425,6 @@ public void testSimpleCursorIteratorLargeMessage() throws Exception { | |||
server = createServer(true, config, PAGE_SIZE, PAGE_MAX); | |||
server.start(); | |||
|
|||
final int numberOfBytes = 200 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method says it is for testing LargeMessage, yet it isnt using the numberOfBytes variable, and doesnt obliviously appear to be using large messages.
Rather than needing removal, this seems more like its indicating a test bug that needs fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sent a new, separate commit to deal with this.
for (Pair<Long, Integer> pair : primaryIds) { | ||
total += pair.getB(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need empty loop this leaves, both here and the similar one that follows immediately.
I would instead wonder what they originally existed to do and whether the test simply isnt/wasnt checking something that it was actually meant to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
|
||
// it should be possible to do it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesnt seem like this should have moved here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
{ | ||
StringBuffer buffer = new StringBuffer(); | ||
for (int i = 0; i < 500; i++) { | ||
buffer.append(" "); | ||
} | ||
bigString = buffer.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable may have been unused before but this change just leaves the buffer behind, itself also still effectively unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most if not all of the changes in this file appear to be in the wrong commit(s), they are in e95f2ac for ARTEMIS 5272 "eliminate unused private fields" but none of the changes in the file do that, they are a mix of using interface type and text block strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now.
// this is not used... I'm only keeping it here because of Serialization compatibility and Wildfly usage on JNDI. | ||
private boolean finalizeCheck; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is strange since, it was only made this way in recent years, but the class wasnt Serializable at the time. The history of the file is littered with references to serialization...but the file was seemingly never Serializable in the time since Artemis existed. There is a nested class in the file that is Serializable though.
Is there perhaps another type of serializing the locator that the earlier change here thought needed to be concious of not breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left this one alone.
// keeping this field for serialization compatibility only. do not use it | ||
private boolean finalizeChecks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar pondering to #5477 (comment), with difference that I can see that the relation to JNDIBaseStorable here probably means this really is actually needed for compatibility and should stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also left this one alone as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably be inclined not to change the environment type in this file (or ReadOnlyContext / TestContext / TestContextFactory), they use Hashtable because Context / InitialContext always use Hashtable for the environment, including in the getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"maxExpiryDelay":4004, | ||
"enableMetrics":false | ||
} | ||
"""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actually an issue, especially for the usage here, but just noting for awareness...the text-block will subtly differ from the original string concats in having a newline at the end, since the closing delimiter is on its own line rather than the same line [and the block also doesnt use the trailing \<line-terminator> escape to suppress a line ending being added when the block is processed].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was aware of the nuance, but I put the """
on a new line for stylistic purposes since it didn't matter to the test(s). I reversed that decision in the most recent push.
doCheckFailureErrorMessageTestImpl(results, """ | ||
org.apache.activemq.artemis.logs.annotation.processor.cases.LAPTCase1_InvalidIDForRegex: \ | ||
Code 100 does not match regular expression specified on the LogBundle: [0-9]{1}\ | ||
"""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think these were all more readable the way they were.
This is actually more verbose, less obviously just supplying the expected message, and requires understanding of the nuanace of text-block escaping as regards to this 4-line text-block actually being a single string without line ending. No real benefit versus the concats, but various downsides.
I'd leave these unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair.
"\nCould not re-establish AMQP Server Connection {} on {} after {} retries" + | ||
"\n*******************************************************************************************************************************\n", level = LogMessage.Level.WARN) | ||
@LogMessage(id = 111001, value = """ | ||
******************************************************************************************************************************* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to remove the deliberate newline at the start?
(same for others)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
private static String firstPart = """ | ||
<core xmlns="urn:activemq:core"> | ||
<name>ActiveMQ.main.config</name> | ||
<log-delegate-factory-class-name>org.apache.activemq.artemis.integration.logging.Log4jLogDelegateFactory</log-delegate-factory-class-name> | ||
<bindings-directory>${jboss.server.data.dir}/activemq/bindings</bindings-directory> | ||
<journal-directory>${jboss.server.data.dir}/activemq/journal</journal-directory> | ||
<journal-min-files>10</journal-min-files> | ||
<large-messages-directory>${jboss.server.data.dir}/activemq/largemessages</large-messages-directory> | ||
<paging-directory>${jboss.server.data.dir}/activemq/paging</paging-directory> | ||
<connectors> | ||
<connector name="netty">tcp://localhost:61616</connector> | ||
<connector name="netty-throughput">tcp://localhost:5545</connector> | ||
<connector name="in-vm">vm://0</connector> | ||
</connectors> | ||
<acceptors> | ||
<acceptor name="netty">tcp://localhost:5545</acceptor> | ||
<acceptor name="netty-throughput">tcp://localhost:5545</acceptor> | ||
<acceptor name="in-vm">vm://0</acceptor> | ||
</acceptors> | ||
<security-settings> | ||
<security-setting match="#"> | ||
<permission type="createNonDurableQueue" roles="guest"/> | ||
<permission type="deleteNonDurableQueue" roles="guest"/> | ||
<permission type="createDurableQueue" roles="guest"/> | ||
<permission type="deleteDurableQueue" roles="guest"/> | ||
<permission type="consume" roles="guest"/> | ||
<permission type="send" roles="guest"/> | ||
</security-setting> | ||
</security-settings> | ||
<address-settings> | ||
<address-setting match="#"> | ||
<dead-letter-address>DLQ</dead-letter-address> | ||
<expiry-address>ExpiryQueue</expiry-address> | ||
<redelivery-delay>0</redelivery-delay> | ||
<max-size-bytes>10485760</max-size-bytes> | ||
<message-counter-history-day-limit>10</message-counter-history-day-limit> | ||
<address-full-policy>BLOCK</address-full-policy> | ||
</address-setting> | ||
</address-settings> | ||
"""; | ||
|
||
private static String lastPart = "</core>"; | ||
|
||
private static String bridgePart = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be safer if final (uppercase might also be nice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Type safety is better with the empty methods (e.g. emptyList, emptyMap, etc.).
appender.console.filter.threshold.level = trace\ | ||
""")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly simpler to add the close deliminator on the same line than use line ending suppression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
appender.console.filter.threshold.level = info\ | ||
""")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly
} else if (value instanceof Boolean boolean1) { | ||
return boolean1; | ||
} else if (value instanceof SimpleString string) { | ||
return Boolean.valueOf(string.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things like boolean1 (plus byte1, short1, float1,and double1 etc that follow below) are more than a bit ugly, probably even less readable compared to the casts.
Calling the SimpleString string results in the silly reading string.toString()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if (run instanceof AtomicRunnable runnable) { | ||
return runnable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (run instanceof AtomicRunnable runnable) { | |
return runnable; | |
if (run instanceof AtomicRunnable atomicRunnable) { | |
return atomicRunnable; |
It is already a Runnable, so calling it merely runnable right after we determined it is more than just that and return it as a different type, simply reads horribly, far more than the original cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if (val instanceof JsonArray jsonArray1) { | ||
Object[] inner = fromJsonArray(jsonArray1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (val instanceof JsonArray jsonArray1) { | |
Object[] inner = fromJsonArray(jsonArray1); | |
if (val instanceof JsonArray jsonArrayValue) { | |
Object[] inner = fromJsonArray(jsonArrayValue); |
if (innerVal instanceof JsonArray jsonArray1) { | ||
innerVal = fromJsonArray(jsonArray1); | ||
} else if (innerVal instanceof JsonString string) { | ||
innerVal = string.getString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (innerVal instanceof JsonArray jsonArray1) { | |
innerVal = fromJsonArray(jsonArray1); | |
} else if (innerVal instanceof JsonString string) { | |
innerVal = string.getString(); | |
if (innerVal instanceof JsonArray innerJsonArray) { | |
innerVal = fromJsonArray(innerJsonArray); | |
} else if (innerVal instanceof JsonString innerJsonString) { | |
innerVal = innerJsonString.getString(); |
} else if (val instanceof JsonString string) { | ||
array[i] = string.getString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if (val instanceof JsonString string) { | |
array[i] = string.getString(); | |
} else if (val instanceof JsonString jsonString) { | |
array[i] = jsonString.getString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm giving up looking at the rest of the json stuff...same type of recent comments+suggestions to make it more readable than originally, rather than arguably less readable, also apply to the rest of it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got all of them.
if (duplicateID instanceof SimpleString string) { | ||
return string.getData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (duplicateID instanceof SimpleString string) { | |
return string.getData(); | |
if (duplicateID instanceof SimpleString simpleString) { | |
return simpleString.getData(); |
@@ -328,18 +328,18 @@ private void encodeMap(final ActiveMQBuffer buffer, final Map<String, Object> ma | |||
|
|||
Object val = entry.getValue(); | |||
|
|||
if (val instanceof Boolean) { | |||
if (val instanceof Boolean boolean1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
booleanVal / longVal / etc style naming or similar , or just leaving the casts, would be more nicer to read later than than boolean1 etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All should be fixed now.
@@ -961,9 +960,9 @@ private int getConnectionWithRetry(final int reconnectAttempts, RemotingConnecti | |||
} | |||
|
|||
if (getConnection() != null) { | |||
if (oldConnection != null && oldConnection instanceof CoreRemotingConnection) { | |||
if (oldConnection != null && oldConnection instanceof CoreRemotingConnection remotingConnection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oldRemotingConnection would be more readable, given its used on the same line as the new CoreRemotingConnection connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if (exception instanceof ActiveMQException qException) { | ||
exception = JMSExceptionHelper.convertFromActiveMQException(qException); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (exception instanceof ActiveMQException qException) { | |
exception = JMSExceptionHelper.convertFromActiveMQException(qException); | |
if (exception instanceof ActiveMQException amqException) { | |
exception = JMSExceptionHelper.convertFromActiveMQException(amqException); |
@@ -634,8 +634,8 @@ public MessageConsumer createSharedConsumer(Topic topic, String name, String mes | |||
} | |||
checkTopic(topic); | |||
ActiveMQTopic localTopic; | |||
if (topic instanceof ActiveMQTopic) { | |||
localTopic = (ActiveMQTopic) topic; | |||
if (topic instanceof ActiveMQTopic qTopic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qException was strange, but qTopic is about the most nonsense thing it could have picked :)
Here and all the others below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qTopic
...lol
if (message instanceof AMQPMessage pMessage) { | ||
return super.invokeInterceptors(this.incomingInterceptors, pMessage, connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (message instanceof AMQPMessage pMessage) { | |
return super.invokeInterceptors(this.incomingInterceptors, pMessage, connection); | |
if (message instanceof AMQPMessage amqpMessage) { | |
return super.invokeInterceptors(this.incomingInterceptors, amqpMessage, connection); |
same below
if (message instanceof AMQPMessage pMessage) { | ||
return AMQPMessageBrokerAccessor.getCurrentProperties(pMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (message instanceof AMQPMessage pMessage) { | |
return AMQPMessageBrokerAccessor.getCurrentProperties(pMessage); | |
if (message instanceof AMQPMessage amqpMessage) { | |
return AMQPMessageBrokerAccessor.getCurrentProperties(amqpMessage); |
if (message instanceof AMQPMessage pMessage) { | ||
return pMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (message instanceof AMQPMessage pMessage) { | |
return pMessage; | |
if (message instanceof AMQPMessage amqpMessage) { | |
return amqpMessage; |
Plus another below
@@ -216,8 +216,8 @@ public static org.apache.activemq.artemis.api.core.Message inbound(final Message | |||
} | |||
|
|||
final Object scheduledDelay = messageSend.getProperties().get(ScheduledMessage.AMQ_SCHEDULED_DELAY); | |||
if (scheduledDelay instanceof Long) { | |||
coreMessage.putLongProperty(org.apache.activemq.artemis.api.core.Message.HDR_SCHEDULED_DELIVERY_TIME, System.currentTimeMillis() + ((Long) scheduledDelay)); | |||
if (scheduledDelay instanceof Long long1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like delayMs or delayLong would be far more readable later.
Just about every name added below is equally unhelpful, and results in less readable code than the original.
if (lv instanceof Comparable comparable && rv instanceof Comparable comparable1) { | ||
return compare(comparable, comparable1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names need updater or its far readable as it originally was.
Same below
if (descriptor instanceof MappedPropertyDescriptor propertyDescriptor) { | ||
if (propertyDescriptor.getMappedWriteMethod() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the leg below could use distinct names, e.g mappedPropertyDescriptor and indexedPropertyDescriptor, to make things more readable. Them both using propertyDescriptor now arguably makes it less readable than originally.
@@ -116,11 +116,11 @@ public Comparator<T> getComparator() { | |||
try { | |||
Object leftValue = getField(left, sortField); | |||
Object rightValue = getField(right, sortField); | |||
if (leftValue instanceof Comparable && rightValue instanceof Comparable) { | |||
if (leftValue instanceof Comparable comparable && rightValue instanceof Comparable comparable1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better names would help here...without them, the code below is far more readable as it was than it is after this adjustment.
if (message != null && message instanceof LargeServerMessage serverMessage) { | ||
serverMessage.setStorageManager(storageManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (message != null && message instanceof LargeServerMessage serverMessage) { | |
serverMessage.setStorageManager(storageManager); | |
if (message != null && message instanceof LargeServerMessage largeServerMessage) { | |
largeServerMessage.setStorageManager(storageManager); |
if (originalRoutingType != null && originalRoutingType instanceof Byte byte1) { | ||
copyMessage.setRoutingType(RoutingType.getType(byte1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (originalRoutingType != null && originalRoutingType instanceof Byte byte1) { | |
copyMessage.setRoutingType(RoutingType.getType(byte1)); | |
if (originalRoutingType != null && originalRoutingType instanceof Byte origRoutingType) { | |
copyMessage.setRoutingType(RoutingType.getType(origRoutingType)); |
Similarly below
@@ -136,15 +136,15 @@ public boolean canInvoke(String name, String operationName) { | |||
|
|||
private void initializeFromFirstServerMBeanRegistration(Method method, Object[] args) { | |||
if (activeMQServer == null && method.getName().equals("registerMBean")) { | |||
if (args != null && args[0] instanceof ActiveMQServerControlImpl) { | |||
activeMQServer = ((ActiveMQServerControlImpl) args[0]).getServer(); | |||
if (args != null && args[0] instanceof ActiveMQServerControlImpl impl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl -> control ?
if (returnMessage instanceof LargeServerMessage serverMessage) { | ||
serverMessage.setStorageManager(storageManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (returnMessage instanceof LargeServerMessage serverMessage) { | |
serverMessage.setStorageManager(storageManager); | |
if (returnMessage instanceof LargeServerMessage largeServerMessage) { | |
largeServerMessage.setStorageManager(storageManager); |
if (message instanceof LargeServerMessage serverMessage) { | ||
serverMessage.setStorageManager(storageManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly
} else if (callback instanceof ClientIDCallback dCallback) { | ||
dCallback.setClientID(remotingConnection.getClientID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if (callback instanceof ClientIDCallback dCallback) { | |
dCallback.setClientID(remotingConnection.getClientID()); | |
} else if (callback instanceof ClientIDCallback clientIDCallback) { | |
clientIDCallback.setClientID(remotingConnection.getClientID()); |
finally done looking through the commits. Left 'a couple' of comments hehe. The last commit however, I didnt comment on lots of things that were just more of the same (I noted where in some cases). For example there are lots of instanceof pattern match name additions like byte1/long1/<other>1, or <last-letter-of-type-prefix>Exception, that make the code far less readable than it was to begin with with the original variable name and a cast. I think the entire commit needs be swept for those and updated before its fit to go in. |
136a2ec
to
536c5cd
Compare
I think I addressed all of your comments 🤞 . |
No description provided.