Skip to content
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

Equals compares collections last #1847

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Equals compares collections last #1847

wants to merge 1 commit into from

Conversation

schlosna
Copy link
Contributor

Before this PR

When evaluating equals(Object), collection types are compared before other relative cheaper fields.

After this PR

==COMMIT_MSG==
Equals compares collections last
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Jul 20, 2022

Generate changelog in changelog/@unreleased

Type
See change types. Select one:

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Equals compares collections last

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -38,6 +40,8 @@

public final class MethodSpecs {

private static final ImmutableSet<String> COLLECTION_TYPES =
ImmutableSet.of("java.util.List", "java.util.Set", "java.util.Map");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could pass through the original conjure type information so that we can avoid regressions if we eventually specialize the field types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could potentially pipe the List<EnrichedField> through and use a DefaultTypeVisitor to determine if field should be treated as a collection. It might be worth piping something like List<Pair<EnrichedField, FieldSpec>> through to ensure proper association between the two.

@schlosna
Copy link
Contributor Author

Right now this is still just a raw WIP I wanted to brain dump as I was digging through a few JFR and saw some conjure types equals methods evaluating java.util.Collections$UnmodifiableSet.equals(Object).

There are some places where we might want to just manually reorder fields in yaml, but was thinking we could defer collection comparison to short-circuit in case of mismatches.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants