Community
Participate
Working Groups
For example, the following conditional compilation pattern produces 'Comparing identical expressions' warnings: public class Test { static final int MODE1 = 1; static final int MODE2 = 2; static final int MODE3 = 3; static final int mode = MODE1; void test() { if (mode == MODE1) {} } } I would propose that the warning should only be flagged if the LHS and RHS refer to the same field (e.g. mode == mode, or mode = Test.mode) - which seems to be implemented separately anyway as it is still flagged if 'mode' is non-final.
What looks obvious and good in this example might be a bug in a similar solution, and drawing the line when this comparison is useful is difficult. The problem is that Java has no concept of conditional compilation and using constants for this purpose cannot be distinguished from using constants in the normal sense. For cases like this it's much easier to set this specific warning to ignore. If this is part of a unit test, please disable this warning for that project or source folder. I see one omission here, though: this option can be disabled via preference (or command line option "compareIdentical"), but there's not @SuppressWarnings token for it. I wonder whether we should add either "compareIdentical" or a more general "redundant" token for suppressing warnigns.
I couldn't (and still can't) think of any situation where this kind of comparison might be a bug. I still think my suggestion would be an all-round improvement, unless you can give me a concrete example of some code that would suffer from it. (Tangentially - "if (mode == MODE2)" doesn't give a warning in my example. The warning only comes up for CONSTANT1==CONSTANT2 if the value is the same. If you want to resolve that inconsistency, then the warning becomes "Warning: comparing two constant fields", or if you prefer, "Warning: [boolean?] expression is compile-time constant". Which I assume you don't think are worthwhile warnings to have [although "Warning: dead code" is available for some of these cases]. But then it's inconsistent, as you're warning about an 'always true' clause but you're not warning about an 'always false' clause.) Incidentally, Java does actually have the concept of conditional compilation. If you check the 'unreachable code' rules for 'if' statements, you'll see that "if (true) return;" for example explicitly allows code after it, even though that code is unreachable, specifically because it's useful for excluding code at compile time. Also, javac (and probably Eclipse's compiler too?) do indeed go ahead and exclude that code from the executable, though I'm not sure if they're required to. I hope I've helped clarify why I think my suggestion is a good idea, and I hope I haven't just created more confusion. :)
(In reply to comment #2) > Incidentally, Java does actually have the concept of conditional > compilation. What I meant is a construct that *only* serves the purpose of selecting parts of the code at compile time, like #ifdef in C. The point is: with #ifdef it is clear that the condition must be a compile-time constant. Using a general purpose 'if' this is not clear. So we can only guess about the intention. As for examples: if (3 == 3) dostuff(); is a bug. final static int THREE = 3; if (THREE == 3) dostuff(); is a bug, too. EnglishLiterals.java: final static int THREE = 3; GermanLiterals.java: final static int DREI = 3; if (EnglishLiterals.THREE == GermanLiterals.DREI) dostuff(); probably is a bug, too. The only difference between my examples and your examples: my constants are intended as fixed constants, whereas your 'mode' is a "variable constant". Sorry, I still don't see where to draw the line. That's why these warnings are configurable / suppressable: so you can adjust them to your code style.
In your example bugs, what do you think they meant to write? I'm failing to see how anyone would accidentally end up with those examples, and what they meant to write instead. Therefore I'm failing to see your argument. For the THREE == DREI example, suppose they wrote THREE == ZWEI instead, that's not going to warn but is equally likely to be wrong. For me, a much more likely example bug is if someone wants "if (this.value == value)" but either forgets the 'this' or hasn't declared the parameter. My original suggestion would still warn in this case.
(In reply to comment #4) > In your example bugs, what do you think they meant to write? I'm failing to > see how anyone would accidentally end up with those examples, and what they > meant to write instead. I don't know and I don't have to know. Use your imagination. Programmers do write nonsense code and expect the compiler to tell them that this is "obviously" nonsense, even if the compiler cannot guess what was intended. For any warnings that somehow relate to programming style it is extremely difficult to find a consensus among *all* Java programmers. This being said I don't believe the issue at hand to be relevant enough to trigger a discussion pro and con a new style of warning. A new token for @SuppressWarnings is comparably cheep, and provides a full solution for your case and many others. Should we add a "redundant" token?
I'm disappointed that you haven't given any examples to support your argument. I continue to believe my suggestion will improve the warning's value without causing any unwanted side-effects or missed bugs. It would be nice if you could ask for second opinions from other JDT developers on this one, but beyond making that request, I won't waste any more of my or your time.
(In reply to comment #0) > For example, the following conditional compilation pattern produces > 'Comparing identical expressions' warnings: > > public class Test { > static final int MODE1 = 1; > static final int MODE2 = 2; > static final int MODE3 = 3; > > static final int mode = MODE1; > > void test() { > if (mode == MODE1) {} > } > } > > I would propose that the warning should only be flagged if the LHS and RHS > refer to the same field (e.g. mode == mode, or mode = Test.mode) - which > seems to be implemented separately anyway as it is still flagged if 'mode' > is non-final. I tried this: public class Test { static final int MODE1 = 1; static final int MODE2 = 2; static final int MODE3 = 3; static int mode = MODE1; void test() { if (mode == MODE1) {} } } And it doesn't report any problem with mode == MODE1 as mode is no longer a constant expression. Are you sure you rebuilt your project after making mode non-final ? Only the comparison of constant expression or the same field can be reported as being a potential problem.
Apologies, my paragraph that you quoted was slightly confusing: > I would propose that the warning should only be flagged if the LHS and RHS > refer to the same field (e.g. mode == mode, or mode = Test.mode) - which > seems to be implemented separately anyway as it is still flagged if 'mode' > is non-final. I didn't mean to say that "mode == MODE1" flags a warning if 'mode' is non-final (it correctly doesn't). I was just explaining how I can tell that the 'same field' check is implemented separately from the 'same constant value' check. My original bug report is essentially that the 'same constant value' check creates false positives for me, but I still want the 'same field' detection to flag a warning. Although thanks for bringing me back to that point, because it highlights the possibility of simply splitting the warnings - you could allow people to independently toggle the warnings for 'comparing field with itself' and 'comparing identical constant values', if you believe there is value in the latter. Or indeed you could replace the second one with 'comparing constant values', and make it equally warn for things like "2 == 3", at which point I can see how there might be people out there who want to detect such expressions. (Maybe this was the source of misunderstanding between me and Stephan? We didn't go into detail about whether "3 == 3" and "2 == 3" should be treated differently for warnings. Currently they are, but why?) Thanks to the new participants, especially as someone mentioned in another bug that you're understaffed.
The code below triggers the warning and there is no way to suppress it. It is really annoying. Words per record is set to 64. Words per line is set to 8. The former must be divisible by the latter, with no remainder. The parameters are set in two different classes. // Checks for typos ... static { // Test for parameter error in line vs record lengths if ((kWordsPerRecord % kWordsPerLine) != 0) { String message = "The number of words per record, " + kWordsPerRecord + "\nmust be exactly divisible by the number of words per line, " + kWordsPerLine; throw new DtssRuntimeException(message); }
Something like @SuppressWarnings("identical-values-comparison") would be nice.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
