Proposal to change `==` comparing constants with `equals`

[tbw777]

Fixed erroneous direct form of comparison ("==") for these object types: String, Integer, Boolean, BigDecimal

Direct comparsion with == operator may cause unpredictable effects for business logic because its skip content check. Single exclusion is using method intern() for String. Replaced with Objects.equals() where left side might be a null and equals() otherwise. No any test directory was changed. This change covers occurrences of the == operator except intern() Also expressions like (== "") changed to IsEmpty()

[BradWalker]

@tbw777,

Ask yourself the following questions:

1 - Did I solve a bug/defect/warning? 2 - Is there a reduction in warning messages? 3 - Is the code broken or just not "perfect" ? 4 - What am I trying to accomplish here?

[tbw777]

@BradWalker

1 - Did I solve a bug/defect/warning? I think it solve bugs

2 - Is there a reduction in warning messages? dont know

3 - Is the code broken or just not "perfect" ? code broken

4 - What am I trying to accomplish here? fix potentional errors

[BradWalker]

@BradWalker

1 - Did I solve a bug/defect/warning? I think it solve bugs

2 - Is there a reduction in warning messages? dont know

3 - Is the code broken or just not "perfect" ? code broken

4 - What am I trying to accomplish here? fix potentional errors

== – checks reference equality

equals() – checks the value equality

In this particular case, it doesn't really matter as they evaluate to the same answer.

[tbw777]

There is no guarantee that the given type located in the pool, so they can be in different objects and == will return false while equals will return true

[BradWalker]

Strings are "interned" as per JLS 3.10.5

So they evaluate to the same thing. Though generally, I would agree, it's better form to use the equals() method.

Again, does this accomplish something or is this "code beautification"?

[tbw777]

@BradWalker As far as I understand, JLS 3.10.5 are talking about compile time constants, not a variables, and in this PR the changes concern not only of the strings

15.28. Constant Expressions A compile-time constant expression

[lkishalmi]

Well, I'm sorry this one is noise.

Many of the changes are touching ancient code written probably for Java 1.4 or earlier. Checking reference equality when the reference is a public constant was usual as these things were treated like enums and maybe for performance reasons,

The Objects class is available from Java 7. Replacing those would improve the readability, I see value in that. I'd create a separate PR with those, maybe that would not touch so many files.

Replacing the ==-s in this one, I fear holds no value.

[tbw777]

@lkishalmi This changes not concern to intern comparing, threfore they are required to be used

[vieiro]

Some questions:

Fixed erroneous direct form of comparison ("==") for these object types: String, Integer, Boolean, BigDecimal

1. Of all the changes you're proposing, which of them do you think are really erroneous?
2. Let's assume NetBeans is working on erroneous code, as you say. Then if we apply all these changes to all these modules, NetBeans will work differently, right? Of all these modules you propose to change, where exactly is going NetBeans to work differently?

There is no guarantee that the given type located in the pool, so they can be in different objects and == will return false while equals will return true

That is a good hint to evaluate the PR, and may help you answer point 2. In which of the modules you propose to change is the type located in the pool? In the enterprise module? Web module? apisupport module? xml module? Java module? ant debugger module? Any other modules?

We have to evaluate the impact of changes in the behavior of NetBeans! What is your opinion?

Thanks, Antonio

[tbw777]

@vieiro I mean data types pool. Pool of JVM, not NB

If you run this code, for example:

    String s1 = "test";
    String s2 = new String("test");

    System.out.println(s1 == s2);
    System.out.println(s1.equals(s2));

A result will:

false true

If somebody want to do pool comparing he must use the intern() method for Strings.

[vieiro]

@vieiro I mean data types pool. Pool of JVM, not NB

If you run this code, for example:

    String s1 = "test";
    String s2 = new String("test");

    System.out.println(s1 == s2);
    System.out.println(s1.equals(s2));

A result will:

false true

If somebody want to do pool comparing he must use the intern() method for Strings.

Exactly. The question is, where is this being done in all the modules you're changing, if any?

[tbw777]

@vieiro This is done in all the modules that could be found. Most likely the coverage of this change is 95-100%. I have not yet found another way to detect such inconsistencies.

A list of changed directories :

• apisupport
• contrib
• enterprise
• extide
• groovy
• ide
• java
• nbbuild
• php
• platform
• profiler

A main target of this PR is bugfix, not cleanup

[vieiro]

@tbw777 So all of these are bugs?

Let me ask the question again:

Let's assume NetBeans is working on erroneous code, as you say. Then if we apply all these changes to all these modules, NetBeans will work differently, right? Of all these modules you propose to change, where exactly is going NetBeans to work differently?

[tbw777]

@vieiro Yes, all these dirs are contains cmp bugs. The main logic of code is correct, but sometimes may dont work if JVM place created new objects without reusage existing equals objects. Its depending on JVM version and JVM vendor.

Tests directories still unchanged but can contains similar bugs. I can add commit or create separete PR for it. Just let me know how to do it the best way.

[neilcsmith-net]

The main logic of code is correct, but sometimes may dont work if JVM place created new objects without reusage existing equals objects. Its depending on JVM version and JVM vendor.

And whether they can be bothered to comply with the Java Language Spec?! String literals are interned.

I don't see an issue in principle with the change, but for each file here we either don't actually have a bug that needs fixing (most likely), or we find an issue where the behaviour does change, in which case we need to verify it's not reliant on the "wrong" behaviour.

[vieiro]

Hi @tbw777 . There goes the first review of the first proposed change. The change you propose will generate a bug in the ModuleDependency class, which may have bad implications for NetBeans.

[tbw777]

Hi @tbw777 . There goes the first review of the first proposed change. The change you propose will generate a bug in the ModuleDependency class, which may have bad implications for NetBeans.

What error do you mean? Explain please.

[vieiro]

I don't see an issue in principle with the change, but for each file here we either don't actually have a bug that needs fixing (most likely), or we find an issue where the behaviour does change, in which case we need to verify it's not reliant on the "wrong" behaviour.

I'm worried about these changes in behaviour, that can open a can of worms with bugs difficult to reproduce. I'm not for merging without reviewing the impact of each change.

[vieiro]

Hi @tbw777 . There goes the first review of the first proposed change. The change you propose will generate a bug in the ModuleDependency class, which may have bad implications for NetBeans.

What error do you mean? Explain please.

It seems you haven't figured out the change in behaviour in NetBeans.

[neilcsmith-net]

@vieiro I meant exactly that - not sure I worded it well. Let's -1 and close this as is then?

A PR for this change, where it's provably a comparison on a String that's possibly not interned, and where that behaviour is wrong, would be good. But then it also might be empty. :smile:

[lkishalmi]

-1

We've already spent too much resources on this one. Closing.

[tbw777]

What are you doing? Its safe bugfix Im stopping to making any new PR while this isnt approved

[lkishalmi]

Well,

I made the call. We are having a discussion on code cleanup PRs here: https://lists.apache.org/thread/f7mt7k3rb11sgjj1fo2vlflczvw3oycx A side discussion also got started here: https://lists.apache.org/thread/k4lhhvxv1jp2oyblgbbn67nghhr63zrr

You claim that this one is a safe bugfix, without naming a bug, that this one would actually fix. @vieiro also mentioned that would cause and error in ModuleDependency class.

Moreover you seem to ignore the raised concerns multiple times. Be humble, do good! Not everything we make has the same goodness value for everyone. I have multiple PRs, that was closed without being merged. The people here, knows, that I can get eager sometimes, rushing for something, then I got pulled back for valid reasons. I'm grateful for that. Sitting back and re-evaluate what are we going for, makes me a better programmer, and I hope a better person as well.

As I've said, I see value in some parts of this PR, but as a whole, this won't sail.

[tbw777]

@lkishalmi @vieiro @neilcsmith-net @BradWalker @mbien I cant quit this changes becouse it really fix bugs! Its not a code clean PR. And I am sure that sometimes they appear in rare cases! How much money do i need to donate (in crypto for example) to finish this PR correctly? May be i need to make some tests? I know that its not easy! Сurrent tests, including ModuleDependency, do not fails! Closing this PR is a mistake because contains bugs refferences ant temporary resolvation!

[neilcsmith-net]

I cant quit this changes becouse it really fix bugs! And I am sure that sometimes they appear in rare cases!

It's only a bug if it's a check against a non-interned String. Please re-read JLS-3.10.5

There are a few things in here where object identity rather than equality might be important.

Try concentrating on cases where object identity isn't needed and the check is a non-interned String - that's potentially a real bug, if you find one.

Some of the other changes might be worth considering if identity is not important, if only to stop having this conversation in future rather than because there's a bug. That probably does need splitting down into multiple small PRs, because it's not code cleanup as you say.

Offering money is pointless (and insulting)! Offer your time, address the concerns, and realise we're all trying to make NetBeans better. Locking this conversation.