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

Remove covered condition in JsonNull.equals() #2271

Conversation

MaicolAntali
Copy link
Contributor

The Condition this == other is covered by subsequent condition other instanceof JsonNull

@MaicolAntali MaicolAntali changed the title Remove already covered condition in JsonNull.equals() Remove covered condition in JsonNull.equals() Dec 6, 2022
@eamonnmcmanus
Copy link
Member

Fair point! Thanks for the contribution.

@eamonnmcmanus eamonnmcmanus merged commit 66d934b into google:master Dec 6, 2022
@Marcono1234
Copy link
Collaborator

One question would be though whether this == other is faster than other instanceof JsonNull. That is relevant because code would mostly work with the JsonNull.INSTANCE singleton. But caring about that might be premature optimization.

@eamonnmcmanus
Copy link
Member

Well, this == other would only be faster when comparing against JsonNull.INSTANCE, right? Whereas I think you might just as often be comparing against instances of other JsonElement classes. Anyway I found only 3 production classes in all of Google's source that did .equals(JsonNull.INSTANCE). Hundreds are doing .isJsonNull(), which sidesteps the question. (To be fair, I also didn't find many Set<JsonElement> or Map<JsonElement, cases either.)

On the whole the performance difference is likely to be absolutely negligible so we might as well simplify.

@MaicolAntali MaicolAntali deleted the remove-covered-condition-JsonNull-equal branch December 9, 2022 10:29
@MaicolAntali MaicolAntali restored the remove-covered-condition-JsonNull-equal branch December 9, 2022 10:29
@MaicolAntali MaicolAntali deleted the remove-covered-condition-JsonNull-equal branch December 9, 2022 10:36
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Jan 20, 2023
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.

3 participants