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

Fix string comparisons #10204

Closed
wants to merge 2 commits into from
Closed

Fix string comparisons #10204

wants to merge 2 commits into from

Conversation

slachiewicz
Copy link
Contributor

No description provided.

@javanna
Copy link
Member

javanna commented Mar 22, 2015

Hi @slachiewicz in this specific case == works just fine, as long as we make sure that TransportResponseHandler#executor always returns exactly one of the constants listed in ThreadPool.Names class, which we use on the other side of the comparison. This is not really a string comparison though, in order for the result to be true the two variables need to reference the same object (the constant ThreadPool.Names.SAME), regardless of whether they contain the same string.

While equals would make a compiler warning go away, and be less error-prone if things change in the future, == is ok here. What do you think?

@slachiewicz
Copy link
Contributor Author

Yes, I agree with You, code works correct and we can only remove compiler warnings.
In NettyTransportTests we have code without any warnings ie. Names.SAME.equals(..) call.
I found also this same warning '==' in LocalTransport class (pull attached).
For me it's ok to close this defect if You not prefer to fix this - i found this while working with code merg for netty 4 update.

@javanna javanna changed the title Fix string comparitions Fix string comparisons Mar 23, 2015
@javanna javanna closed this in 341a52d Mar 23, 2015
javanna pushed a commit that referenced this pull request Mar 23, 2015
…sons

Although the current comparisons work, since we rely on the fact that we always use the same constants, `equals` seems safer.

Closes #10204
@javanna
Copy link
Member

javanna commented Mar 23, 2015

Thanks @slachiewicz merged.

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

Successfully merging this pull request may close these issues.

None yet

3 participants