-
Notifications
You must be signed in to change notification settings - Fork 9
Apply Qodana cleanup fixes for qodana-cleanup #628
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
Apply Qodana cleanup fixes for qodana-cleanup #628
Conversation
Summary of ChangesHello @andrii0lomakin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request incorporates automated code cleanup fixes suggested by Qodana, primarily focusing on modernizing the codebase by adopting Java record types for various internal classes. It also includes adding missing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Qodana for JVM51 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
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.
Code Review
This pull request applies automated Qodana cleanup fixes, primarily converting simple data-holding classes to Java records. This is a great improvement for code conciseness and maintainability. The changes are mostly correct, but I've found a critical bug in one of the equals methods and several formatting issues in the newly created records. Please see the detailed comments.
| @@ -31,10 +15,10 @@ public boolean equals(Object o) { | |||
|
|
|||
| var oRawPair = (RawPairIntegerObject<?>) o; | |||
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.
There is a critical bug in the equals method. It incorrectly casts the object o to RawPairIntegerObject instead of RawPairLongObject. This will cause a ClassCastException at runtime when comparing two RawPairLongObject instances. Please correct the cast.
| var oRawPair = (RawPairIntegerObject<?>) o; | |
| var oRawPair = (RawPairLongObject<?>) o; |
| public void lock() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public void lockInterruptibly() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| @Override | ||
| public void lockInterruptibly() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean tryLock() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| @Override | ||
| public boolean tryLock() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean tryLock(long time, TimeUnit unit) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| @Override | ||
| public boolean tryLock(long time, TimeUnit unit) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public void unlock() { | ||
| if (readLock) { | ||
| spinLock.releaseReadLock(); | ||
| } else { | ||
| spinLock.releaseWriteLock(); | ||
| @Override | ||
| public void unlock() { | ||
| if (readLock) { | ||
| spinLock.releaseReadLock(); | ||
| } else { | ||
| spinLock.releaseWriteLock(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Condition newCondition() { | ||
| throw new UnsupportedOperationException(); | ||
| @Override | ||
| public Condition newCondition() { | ||
| throw new UnsupportedOperationException(); | ||
| } |
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 methods within the SpinLockWrapper record are incorrectly indented. This affects readability and consistency with standard Java formatting conventions. Please remove the extra indentation.
public void lock() {
throw new UnsupportedOperationException();
}
@Override
public void lockInterruptibly() {
throw new UnsupportedOperationException();
}
@Override
public boolean tryLock() {
throw new UnsupportedOperationException();
}
@Override
public boolean tryLock(long time, TimeUnit unit) {
throw new UnsupportedOperationException();
}
@Override
public void unlock() {
if (readLock) {
spinLock.releaseReadLock();
} else {
spinLock.releaseWriteLock();
}
}
@Override
public Condition newCondition() {
throw new UnsupportedOperationException();
}| private static final class PointerTracker { | ||
|
|
||
| private final Exception allocation; | ||
| * Holder which contains if memory tracking is enabled stack trace for the first allocation. |
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 Javadoc for the PointerTracker record has extra leading spaces, which affects formatting. Please remove the unnecessary indentation.
| * Holder which contains if memory tracking is enabled stack trace for the first allocation. | |
| * Holder which contains if memory tracking is enabled stack trace for the first allocation. |
| public void run() { | ||
| final var accumulator = new EnumMap<Intention, ModifiableLong>(Intention.class); | ||
|
|
||
| for (final var consumptionMap : consumptionMaps) { | ||
| accumulateConsumptionStatistics(accumulator, consumptionMap); | ||
| } | ||
| for (final var consumptionMap : consumptionMaps) { | ||
| accumulateConsumptionStatistics(accumulator, consumptionMap); | ||
| } | ||
|
|
||
| final var memoryStat = printMemoryStatistics(accumulator); | ||
| LogManager.instance().info(this, memoryStat); | ||
| final var memoryStat = printMemoryStatistics(accumulator); | ||
| LogManager.instance().info(this, memoryStat); | ||
| } |
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 run method within the MemoryStatPrinter record is incorrectly indented. Please correct the formatting for better readability and consistency.
| public void run() { | |
| final var accumulator = new EnumMap<Intention, ModifiableLong>(Intention.class); | |
| for (final var consumptionMap : consumptionMaps) { | |
| accumulateConsumptionStatistics(accumulator, consumptionMap); | |
| } | |
| for (final var consumptionMap : consumptionMaps) { | |
| accumulateConsumptionStatistics(accumulator, consumptionMap); | |
| } | |
| final var memoryStat = printMemoryStatistics(accumulator); | |
| LogManager.instance().info(this, memoryStat); | |
| final var memoryStat = printMemoryStatistics(accumulator); | |
| LogManager.instance().info(this, memoryStat); | |
| } | |
| public void run() { | |
| final var accumulator = new EnumMap<Intention, ModifiableLong>(Intention.class); | |
| for (final var consumptionMap : consumptionMaps) { | |
| accumulateConsumptionStatistics(accumulator, consumptionMap); | |
| } | |
| final var memoryStat = printMemoryStatistics(accumulator); | |
| LogManager.instance().info(this, memoryStat); | |
| } |
| public boolean tryAdvance(Consumer<? super RawPair<RID, Change>> action) { | ||
| return spliterator.tryAdvance( | ||
| entry -> action.accept(new RawPair<>(entry.getKey(), entry.getValue()))); | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public Spliterator<RawPair<RID, Change>> trySplit() { | ||
| var split = spliterator.trySplit(); | ||
| return split != null ? new TransformingSpliterator(split) : null; | ||
| } | ||
|
|
||
| @Override | ||
| public long estimateSize() { | ||
| return spliterator.estimateSize(); | ||
| } | ||
|
|
||
| @Override | ||
| public int characteristics() { | ||
| return spliterator.characteristics(); | ||
| } |
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 methods inside the TransformingSpliterator record are incorrectly indented. This should be fixed to improve code readability and adhere to standard Java formatting conventions.
public boolean tryAdvance(Consumer<? super RawPair<RID, Change>> action) {
return spliterator.tryAdvance(
entry -> action.accept(new RawPair<>(entry.getKey(), entry.getValue())));
}
@Override
@Nullable
public Spliterator<RawPair<RID, Change>> trySplit() {
var split = spliterator.trySplit();
return split != null ? new TransformingSpliterator(split) : null;
}
@Override
public long estimateSize() {
return spliterator.estimateSize();
}
@Override
public int characteristics() {
return spliterator.characteristics();
}| public boolean tryAdvance(Consumer<? super ObjectIntPair<RID>> action) { | ||
| return delegate.tryAdvance(pair -> { | ||
| final var rid = new RecordId(pair.first().targetCollection(), | ||
| pair.first().targetPosition()); | ||
| action.accept(new ObjectIntImmutablePair<>(rid, pair.second())); | ||
| }); | ||
| } | ||
|
|
||
| @Nullable | ||
| @Override | ||
| public Spliterator<ObjectIntPair<RID>> trySplit() { | ||
| return new TransformingSpliterator(delegate.trySplit()); | ||
| } | ||
| @Nullable | ||
| @Override | ||
| public Spliterator<ObjectIntPair<RID>> trySplit() { | ||
| return new TransformingSpliterator(delegate.trySplit()); | ||
| } | ||
|
|
||
| @Override | ||
| public long estimateSize() { | ||
| return delegate.estimateSize(); | ||
| } | ||
| @Override | ||
| public long estimateSize() { | ||
| return delegate.estimateSize(); | ||
| } | ||
|
|
||
| @Override | ||
| public int characteristics() { | ||
| return delegate.characteristics(); | ||
| @Override | ||
| public int characteristics() { | ||
| return delegate.characteristics(); | ||
| } |
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 methods inside the TransformingSpliterator record are incorrectly indented. Please fix the formatting for consistency and readability.
public boolean tryAdvance(Consumer<? super ObjectIntPair<RID>> action) {
return delegate.tryAdvance(pair -> {
final var rid = new RecordId(pair.first().targetCollection(),
pair.first().targetPosition());
action.accept(new ObjectIntImmutablePair<>(rid, pair.second()));
});
}
@Nullable
@Override
public Spliterator<ObjectIntPair<RID>> trySplit() {
return new TransformingSpliterator(delegate.trySplit());
}
@Override
public long estimateSize() {
return delegate.estimateSize();
}
@Override
public int characteristics() {
return delegate.characteristics();
}| public DatabaseSessionInternal getThreadDatabase() { | ||
| return context.cachedPool(database, username, password).acquire(); | ||
| } |
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 getThreadDatabase method inside the TestScheduleDatabaseFactory record is incorrectly indented. Please correct the formatting.
| public DatabaseSessionInternal getThreadDatabase() { | |
| return context.cachedPool(database, username, password).acquire(); | |
| } | |
| public DatabaseSessionInternal getThreadDatabase() { | |
| return context.cachedPool(database, username, password).acquire(); | |
| } |
Summary
Test plan
🤖 Generated automatically by Qodana workflow