Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

introduce the notion of equivalence #173

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 14, 2022

Resolves #158

@Ladicek Ladicek added this to the 3.0.0 milestone Feb 14, 2022
@Ladicek Ladicek linked an issue Feb 14, 2022 that may be closed by this pull request
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 15, 2022

FYI @mkouba @FroMage, this should be pretty useful for anyone developing an annotation overlay on top of Jandex.

I'm not merging this yet, because I'd like to hear from you, and also because I really should add a few more tests :-)


@Override
public int hashCode() {
int result = Objects.hash(className, returnType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perf experts always say that Objects.hash() is evil when used in hot paths because it allocates one more unnecessary array ;-). In fact, it also sometimes helps to compute the hashcode in the constructor and save it in a field.

It depends on whether we expect the equivalence keys in hash maps and in hot paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ignoring this for now. If that ever becomes problematic, it's straightforward to improve.

Copy link
Contributor

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Looks useful, yes. But not really an annotation override ;)

@FroMage
Copy link
Contributor

FroMage commented Feb 15, 2022

Is it intentional that this is targeted at the smallrye branch and not main?

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 28, 2022

Not an overlay per se, but having built an annotation overlay on top of Jandex myself, I find this a rather important building block.

I didn't consider putting this into 2.4 (would probably become 2.5?), I always thought of this as 3.0 material, but there's probably nothing preventing inclusion into 2.x.

@Ladicek Ladicek force-pushed the equivalence-key branch 2 times, most recently from 43aa60c to 9f74dbf Compare February 28, 2022 17:06
@Ladicek Ladicek merged commit aaa2f2f into smallrye:smallrye Mar 1, 2022
@Ladicek Ladicek deleted the equivalence-key branch March 1, 2022 16:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the notion of equivalence
3 participants